[PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af

[PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af

[Lin Ma]

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(-)

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

Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af

[Jakub Kicinski]

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

Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af

[Lin Ma]

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

Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af

[Jeremy Kerr]

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

Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af

[Lin Ma]

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