In mctp set_link_af implementation `mctp_set_link_af`, it uses strict
parser nla_parse_nested to parse the nested attribute. This is fine in
most cases but not here, as the rtnetlink uses *bad magic* in setlink
code, see code snippet in function `do_setlink`.
nla_for_each_nested(af, tb[IFLA_AF_SPEC], rem) {
const struct rtnl_af_ops *af_ops;
BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af)))); <= (1)
err = af_ops->set_link_af(dev, af, extack); <= (2)
That is, in line (1), the attribute type of af will used to look up the
af_ops, and for MCTP case will use AF_MCTP here to get mctp_af_ops.
Therefore, the attribute with type AF_MCTP will never survive in the
check within the nla_parse_nested.
if (!(nla->nla_type & NLA_F_NESTED)) { <= nla_type is AF_MCTP
NL_SET_ERR_MSG_ATTR(extack, nla, "NLA_F_NESTED is missing");
return -EINVAL; <= always invalid
}
For other set_link_af users IPV4 and IPV6 both make a trick here by
using nla_parse_nested_deprecated, which will check the NLA_F_NESTED
then able to use this type field as family value. This patch simply port
the MCTP code also to deprecated parser to make it work.
Signed-off-by: Lin Ma <[email protected]>
---
net/mctp/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mctp/device.c b/net/mctp/device.c
index acb97b257428..226c8e3ed85f 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -400,8 +400,8 @@ static int mctp_set_link_af(struct net_device *dev, const struct nlattr *attr,
struct mctp_dev *mdev;
int rc;
- rc = nla_parse_nested(tb, IFLA_MCTP_MAX, attr, ifla_af_mctp_policy,
- NULL);
+ rc = nla_parse_nested_deprecated(tb, IFLA_MCTP_MAX, attr, ifla_af_mctp_policy,
+ NULL);
if (rc)
return rc;
--
2.17.1
On Thu, 28 Dec 2023 15:02:58 +0800 Lin Ma wrote:
> In mctp set_link_af implementation `mctp_set_link_af`, it uses strict
> parser nla_parse_nested to parse the nested attribute. This is fine in
> most cases but not here, as the rtnetlink uses *bad magic* in setlink
> code, see code snippet in function `do_setlink`.
>
> nla_for_each_nested(af, tb[IFLA_AF_SPEC], rem) {
> const struct rtnl_af_ops *af_ops;
> BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af)))); <= (1)
> err = af_ops->set_link_af(dev, af, extack); <= (2)
>
> That is, in line (1), the attribute type of af will used to look up the
> af_ops, and for MCTP case will use AF_MCTP here to get mctp_af_ops.
> Therefore, the attribute with type AF_MCTP will never survive in the
> check within the nla_parse_nested.
>
> if (!(nla->nla_type & NLA_F_NESTED)) { <= nla_type is AF_MCTP
> NL_SET_ERR_MSG_ATTR(extack, nla, "NLA_F_NESTED is missing");
> return -EINVAL; <= always invalid
> }
>
> For other set_link_af users IPV4 and IPV6 both make a trick here by
> using nla_parse_nested_deprecated, which will check the NLA_F_NESTED
> then able to use this type field as family value. This patch simply port
> the MCTP code also to deprecated parser to make it work.
Did you test this? It's a suspiciously detailed and yet seemingly
incorrect explanation..
--
pv-bot: s
pw-bot: cr
Hello Jakub,
> >
> > For other set_link_af users IPV4 and IPV6 both make a trick here by
> > using nla_parse_nested_deprecated, which will check the NLA_F_NESTED
> > then able to use this type field as family value. This patch simply port
> > the MCTP code also to deprecated parser to make it work.
>
> Did you test this? It's a suspiciously detailed and yet seemingly
> incorrect explanation..
Yes, I hacked the iproute tool and add below code
https://github.com/iproute2/iproute2/blob/main/ip/iplink.c#L801
853a854,861
> } else if (matches(*argv, "mctptest") == 0) {
> struct rtattr *afs, *afsmctp;
> afs = addattr_nest(&req->n, sizeof(*req), IFLA_AF_SPEC);
> afsmctp = addattr_nest(&req->n, sizeof(*req), AF_MCTP);
> addattr32(&req->n, sizeof(*req),
> IFLA_MCTP_NET, 0);
> addattr_nest_end(&req->n, afsmctp);
> addattr_nest_end(&req->n, afs);
according to the implementation of the `addrgenmode`.
In short, the package format should be like
L T L T
+---+--------------+---+---------+-------------
| | IFLA_AF_SPEC | | AF_MCTP | .....
+---+--------------+---+---------+-------------
^
|
af
Therefore, the code
nla_for_each_nested(af, tb[IFLA_AF_SPEC], rem) {
const struct rtnl_af_ops *af_ops;
BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af)))); <= (1)
err = af_ops->set_link_af(dev, af, extack); <= (2)
(1) will get AF_MCTP with nla_type(...) call, and (2) will then passing `af` to
`mctp_set_link_af`, which will expect the `NLA_F_NESTED` flag.
With the hacked iproute tools, simple type command like
`ip link set eth0 mtu 1024 mctptest` will hit the function mctp_set_link_af and
with my debugger, I found that the nla_parse_nested always return error.
Another elegant solution is to change how (1) calls rtnl_af_lookup, we can introduce
another family attribute and avoid to use nla_type(..) which is so suspicious.
But this will have to change also other set_link_af implementations (ipv4, ipv6).
> --
> pv-bot: s
> pw-bot: cr
Regards
Lin
Hi Lin,
> (1) will get AF_MCTP with nla_type(...) call, and (2) will then
> passing `af` to `mctp_set_link_af`, which will expect the
> `NLA_F_NESTED` flag.
You could set that flag? It's not exclusive with the attribute type.
From netlink.h:
/*
* nla_type (16 bits)
* +---+---+-------------------------------+
* | N | O | Attribute Type |
* +---+---+-------------------------------+
* N := Carries nested attributes
* O := Payload stored in network byte order
*
* Note: The N and O flag are mutually exclusive.
*/
Which is what we do with existing userspace:
https://github.com/CodeConstruct/mctp/blob/6aa4b052d/src/mctp.c#L569C1-L569C1
Cheers,
Jeremy
Hello Jeremy,
>
> Hi Lin,
>
> > (1) will get AF_MCTP with nla_type(...) call, and (2) will then
> > passing `af` to `mctp_set_link_af`, which will expect the
> > `NLA_F_NESTED` flag.
>
> You could set that flag? It's not exclusive with the attribute type.
> From netlink.h:
>
> /*
> * nla_type (16 bits)
> * +---+---+-------------------------------+
> * | N | O | Attribute Type |
> * +---+---+-------------------------------+
> * N := Carries nested attributes
> * O := Payload stored in network byte order
> *
> * Note: The N and O flag are mutually exclusive.
> */
>
> Which is what we do with existing userspace:
>
> https://github.com/CodeConstruct/mctp/blob/6aa4b052d/src/mctp.c#L569C1-L569C1
>
> Cheers,
Oops, my bad. Just know that the nla_type will mask with `NLA_TYPE_MASK`.
Sorry for the bother. T^T
>
>
> Jeremy
Thanks
Lin