| Message ID | 20231228070258.3052422-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] net: mctp: use deprecated parser in mctp_set_link_af | expand |
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..
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
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;
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 <linma@zju.edu.cn> --- net/mctp/device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)