linux-kernel - Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7cbc3044.688a7.18cd32c5f14.Coremail.linma@zju.edu.cn>
Date: Thu, 4 Jan 2024 14:32:29 +0800 (GMT+08:00)
From: "Lin Ma" <linma@....edu.cn>
To: "Jakub Kicinski" <kuba@...nel.org>
Cc: jk@...econstruct.com.au, matt@...econstruct.com.au, davem@...emloft.net, 
	edumazet@...gle.com, pabeni@...hat.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v1] net: mctp: use deprecated parser in
 mctp_set_link_af

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