Message ID | 20230731121247.3972783-1-linma@zju.edu.cn |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:918b:0:b0:3e4:2afc:c1 with SMTP id s11csp1996113vqg; Mon, 31 Jul 2023 06:01:01 -0700 (PDT) X-Google-Smtp-Source: APBJJlHQ4l83apkvvOixeVfUVn6Z//3VXFuYlkumA3jzJgsf1dzV34CEMCU85JTHOZhub3w9vZek X-Received: by 2002:a05:6512:3ba9:b0:4fe:ec5:ebb5 with SMTP id g41-20020a0565123ba900b004fe0ec5ebb5mr7695066lfv.58.1690808460766; Mon, 31 Jul 2023 06:01:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690808460; cv=none; d=google.com; s=arc-20160816; b=SvTbJ6hvPjYZDg8csLjAtM1FqYxID3FtgYIYFMpIhdNJa/Wf/u2+pwhZEYpieuzM2P doX7OysqTFoHPHl2Dif3BWrLOgGy401IiLiLGE6v76yCwVDKtxFMI+vcJf5lR9xOkgMO cr8pvF3yQvDNdd8s0QuuI28iqcVng+YDVQpmBediYwaqMYNPkish5Q4WYtPLPIX1iXQ0 qHIvGHLjS+Nu/IG1MSSFdqWAux/hFOHTFfp7QeQ2AxYR2cOiy7c0PVoCwIf1yrzPYu5k P+qkjvaqQ59dnVvptEGcoql0pd1QeaPbcZvyMaNkicqNfWK3U3TCmAowHZQVT5j/upVm vLAQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:to:from; bh=M1ORbqEh0bVTjGta2/PLRtIW1MdddSYl4ZTPyYXL6SU=; fh=iiRMkQV9Qsmq5ccdcnx7Z8UynCdxCzxcOD536FcAHag=; b=S06GsXAZ28FpgzGuSgRzuaEZoUb0uZ4I9TODUFni8o1vCRdOUDMI1K0B6hI2v+ZaOT lsevxSsZtbfUla2N70xVceBln32BTAHlNwXHmzp12ijXxAk+yyp5TMTPsgbZVluBjhjn YgGAIBNIhdNGnKrMob7GV7QYHPMoeWxzjCmx2zySZM2gkwo46Pj5jzHWtX6bKaXUQZqP 6ZwBLi/hCscNX6Qy06ex/VWMpo2nrlgZlV8BycSdP7t79Vzb8J1SXTy09AS1mHIJDDZx oCxbJiJBa3ItngyxTHj9DhpL3h5cb+1suGunTZvEjXfiYqj8jLnmhf76ysRi1R2MR9Zm ZfMA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id kj27-20020a170907765b00b00994b9c51cafsi2884136ejc.717.2023.07.31.06.00.33; Mon, 31 Jul 2023 06:01:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229570AbjGaMNc (ORCPT <rfc822;dengxinlin2429@gmail.com> + 99 others); Mon, 31 Jul 2023 08:13:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38130 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229713AbjGaMNa (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 31 Jul 2023 08:13:30 -0400 Received: from zg8tndyumtaxlji0oc4xnzya.icoremail.net (zg8tndyumtaxlji0oc4xnzya.icoremail.net [46.101.248.176]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 74D0D10DF; Mon, 31 Jul 2023 05:13:27 -0700 (PDT) Received: from localhost.localdomain (unknown [125.120.146.22]) by mail-app2 (Coremail) with SMTP id by_KCgDHP8NMpcdkbRbaCg--.54964S4; Mon, 31 Jul 2023 20:13:01 +0800 (CST) From: Lin Ma <linma@zju.edu.cn> To: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, fw@strlen.de, yang.lee@linux.alibaba.com, jgg@ziepe.ca, markzhang@nvidia.com, phaddad@nvidia.com, yuancan@huawei.com, linma@zju.edu.cn, ohartoov@nvidia.com, chenzhongjin@huawei.com, aharonl@nvidia.com, leon@kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org Subject: [PATCH net v1 1/2] netlink: let len field used to parse type-not-care nested attrs Date: Mon, 31 Jul 2023 20:12:47 +0800 Message-Id: <20230731121247.3972783-1-linma@zju.edu.cn> X-Mailer: git-send-email 2.17.1 X-CM-TRANSID: by_KCgDHP8NMpcdkbRbaCg--.54964S4 X-Coremail-Antispam: 1UD129KBjvJXoWxZr18JF45tF15Kw17GryUWrg_yoWrGFW5pF Wvkryjyr9xGryxCr92kr1Iga4aqr18JrZ8GrZ8Xws7ZFs0g3srG34rWFnIva4I9F48Ja17 tF1YgrW3uF1UZ37anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUkE14x267AKxVW5JVWrJwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26w1j6s0DM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26F4U JVW0owA2z4x0Y4vEx4A2jsIE14v26rxl6s0DM28EF7xvwVC2z280aVCY1x0267AKxVW0oV Cq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0 I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r 4UM4x0Y48IcxkI7VAKI48JM4x0x7Aq67IIx4CEVc8vx2IErcIFxwACI402YVCY1x02628v n2kIc2xKxwCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c02F4 0E14v26r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_GFv_Wryl IxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7CjxV AFwI0_Gr0_Cr1lIxAIcVCF04k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2jsIE14v26r1j 6r4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UYxBIdaVFxhVjvjDU0xZFpf9x0JUQvt AUUUUU= X-CM-SenderInfo: qtrwiiyqvtljo62m3hxhgxhubq/ X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772941172058052523 X-GMAIL-MSGID: 1772941172058052523 |
Series |
[net,v1,1/2] netlink: let len field used to parse type-not-care nested attrs
|
|
Commit Message
Lin Ma
July 31, 2023, 12:12 p.m. UTC
Recently I found several manual parsing cases for nested attributes
whose fix is rather trivial. The pattern for those like below
const struct nla_policy y[...] = {
...
X = { .type = NLA_NESTED },
...
}
nla_for_each_nested/attr(nla, tb[X], ...) {
// nla_type never used
...
x = nla_data(nla) // directly access nla without length checking
....
}
One example can be found in discussion at:
https://lore.kernel.org/all/20230723074504.3706691-1-linma@zju.edu.cn/
In short, the very direct idea to fix such lengh-check-forgotten bug is
add nla_len() checks like
if (nla_len(nla) < SOME_LEN)
return -EINVAL;
However, this is tedious and just like Leon said: add another layer of
cabal knowledge. The better solution should leverage the nla_policy and
discard nlattr whose length is invalid when doing parsing. That is, we
should defined a nested_policy for the X above like
X = { NLA_POLICY_NESTED(Z) },
But unfortunately, as said above, the nla_type is never used in such
manual parsing cases, which means is difficult to defined a nested
policy Z without breaking user space (they may put weird value in type
of these nlattrs, we have no idea).
To this end, this commit uses the len field in nla_policy crafty and
allow the existing validate_nla checks such type-not-care nested attrs.
In current implementation, for attribute with type NLA_NESTED, the len
field used as the length of the nested_policy:
{ .type = NLA_NESTED, .nested_policy = policy, .len = maxattr }
_NLA_POLICY_NESTED(ARRAY_SIZE(policy) - 1, policy)
If one nlattr does not provide policy, like the example X, this len
field is not used. This means we can leverage this field for our end.
This commit introduces one new macro named NLA_POLICY_NESTED_NO_TYPE
and let validate_nla() to use the len field as a hint to check the
nested attributes. Therefore, such manual parsing code can also define
a nla_policy and take advantage of the validation within the existing
parsers like nla_parse_deprecated..
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
include/net/netlink.h | 6 ++++++
lib/nlattr.c | 11 +++++++++++
2 files changed, 17 insertions(+)
Comments
On Mon, 31 Jul 2023 20:12:47 +0800 Lin Ma wrote: > In short, the very direct idea to fix such lengh-check-forgotten bug is > add nla_len() checks like > > if (nla_len(nla) < SOME_LEN) > return -EINVAL; > > However, this is tedious and just like Leon said: add another layer of > cabal knowledge. The better solution should leverage the nla_policy and > discard nlattr whose length is invalid when doing parsing. That is, we > should defined a nested_policy for the X above like Hard no. Putting array index into attr type is an advanced case and the parsing code has to be able to deal with low level netlink details. Higher level API should remove the nla_for_each_nested() completely which is rather hard to achieve here. Nacked-by: Jakub Kicinski <kuba@kernel.org>
Hello Jakub, > > > > However, this is tedious and just like Leon said: add another layer of > > cabal knowledge. The better solution should leverage the nla_policy and > > discard nlattr whose length is invalid when doing parsing. That is, we > > should defined a nested_policy for the X above like > > Hard no. Putting array index into attr type is an advanced case and the > parsing code has to be able to deal with low level netlink details. Well, I just known that the type field for those attributes is used as array index. Hence, for this advanced case, could we define another NLA type, maybe NLA_NESTED_IDXARRAY enum? That may be much clearer against modifying existing code. > Higher level API should remove the nla_for_each_nested() completely > which is rather hard to achieve here. By investigating the code uses nla_for_each_nested macro. There are basically two scenarios: 1. manually parse nested attributes whose type is not cared (the advance case use type as index here). 2. manually parse nested attributes for *one* specific type. Such code do nla_type check. From the API side, to completely remove nla_for_each_nested and avoid the manual parsing. I think we can choose two solutions. Solution-1: add a parsing helper that receives a function pointer as an argument, it will call this pointer after carefully verify the type and length of an attribute. Solution-2: add a parsing helper that traverses this nested twice, the first time to do counting size for allocating heap buffer (or stack buffer from the caller if the max count is known). The second time to fill this buffer with attribute pointers. Which one is preferred? Please enlighten me about this and I can try to propose a fix. (I personally like the solution-2 as it works like the existing parsers like nla_parse) > > Nacked-by: Jakub Kicinski <kuba@kernel.org> Thanks Lin
On Tue, 1 Aug 2023 10:00:01 +0800 (GMT+08:00) Lin Ma wrote: > > > However, this is tedious and just like Leon said: add another layer of > > > cabal knowledge. The better solution should leverage the nla_policy and > > > discard nlattr whose length is invalid when doing parsing. That is, we > > > should defined a nested_policy for the X above like > > > > Hard no. Putting array index into attr type is an advanced case and the > > parsing code has to be able to deal with low level netlink details. > > Well, I just known that the type field for those attributes is used as array > index. > Hence, for this advanced case, could we define another NLA type, maybe > NLA_NESTED_IDXARRAY enum? That may be much clearer against modifying existing > code. What if the value is of a complex type (nest)? For 10th time, if someone does "interesting things" they must know what they're doing. > > Higher level API should remove the nla_for_each_nested() completely > > which is rather hard to achieve here. > > By investigating the code uses nla_for_each_nested macro. There are basically > two scenarios: > > 1. manually parse nested attributes whose type is not cared (the advance case > use type as index here). > 2. manually parse nested attributes for *one* specific type. Such code do > nla_type check. > > From the API side, to completely remove nla_for_each_nested and avoid the > manual parsing. I think we can choose two solutions. > > Solution-1: add a parsing helper that receives a function pointer as an > argument, it will call this pointer after carefully verify the > type and length of an attribute. > > Solution-2: add a parsing helper that traverses this nested twice, the first > time to do counting size for allocating heap buffer (or stack > buffer from the caller if the max count is known). The second > time to fill this buffer with attribute pointers. > > Which one is preferred? Please enlighten me about this and I can try to propose > a fix. (I personally like the solution-2 as it works like the existing parsers > like nla_parse) Your initial fix was perfectly fine. We do need a solution for a normal multi-attr parse, but that's not the case here.
Hello Jakub, > > Your initial fix was perfectly fine. > > We do need a solution for a normal multi-attr parse, but that's not > the case here. Cool 😎 Thanks Lin
On Mon, Jul 31, 2023 at 12:03:26PM -0700, Jakub Kicinski wrote: > On Mon, 31 Jul 2023 20:12:47 +0800 Lin Ma wrote: > > In short, the very direct idea to fix such lengh-check-forgotten bug is > > add nla_len() checks like > > > > if (nla_len(nla) < SOME_LEN) > > return -EINVAL; > > > > However, this is tedious and just like Leon said: add another layer of > > cabal knowledge. The better solution should leverage the nla_policy and > > discard nlattr whose length is invalid when doing parsing. That is, we > > should defined a nested_policy for the X above like > > Hard no. Putting array index into attr type is an advanced case and the > parsing code has to be able to deal with low level netlink details. Jakub, IMHO, you are lowering too much the separation line between simple vs. advanced use cases. I had no idea that my use-case of passing nested netlink array is counted as advanced usage. Thanks
On Tue, 1 Aug 2023 11:11:17 +0300 Leon Romanovsky wrote: > IMHO, you are lowering too much the separation line between simple vs. > advanced use cases. > > I had no idea that my use-case of passing nested netlink array is counted > as advanced usage. Agreed, that's a fair point. I'm guessing it was inspired by the ethtool stats? (Which in hindsight were a mistake on my part.) For the longest time there was no docs or best practices for netlink. We have the documentation and more infrastructure in place now. I hope if you wrote the code today the distinction would have been clearer. If we start adding APIs for various one-(two?)-offs from the past we'll never dig ourselves out of the "no idea what's the normal use of these APIs" hole..
On Tue, Aug 01, 2023 at 10:57:26AM -0700, Jakub Kicinski wrote: > On Tue, 1 Aug 2023 11:11:17 +0300 Leon Romanovsky wrote: > > IMHO, you are lowering too much the separation line between simple vs. > > advanced use cases. > > > > I had no idea that my use-case of passing nested netlink array is counted > > as advanced usage. > > Agreed, that's a fair point. I'm guessing it was inspired by the > ethtool stats? (Which in hindsight were a mistake on my part.) I don't remember which part of kernel can be blamed for it. :) > > For the longest time there was no docs or best practices for netlink. > We have the documentation and more infrastructure in place now. > I hope if you wrote the code today the distinction would have been > clearer. > > If we start adding APIs for various one-(two?)-offs from the past > we'll never dig ourselves out of the "no idea what's the normal use > of these APIs" hole.. I agree with this sentence, just afraid that it is unrealistic goal, due to extensive flexibility in netlink UAPI toward user-space, which allows you to shoot yourself in the foot without even noticing it. Thanks
Hello there, > For the longest time there was no docs or best practices for netlink. > We have the documentation and more infrastructure in place now. > I hope if you wrote the code today the distinction would have been > clearer. > > If we start adding APIs for various one-(two?)-offs from the past > we'll never dig ourselves out of the "no idea what's the normal use > of these APIs" hole.. This is true. Actually, those check missing codes are mostly old codes and modern netlink consumers will choose the general netlink interface which can automatically get attributes description from YAML and never need to do things like *manual parsing* anymore. However, according to my practice in auditing the code, I found there are some general netlink interface users confront other issues like choosing GENL_DONT_VALIDATE_STRICT without thinking or forgetting add a new nla_policy when introducing new attributes. To this end, I'm currently writing a simple documentation about Netlink interface best practices for the kernel developer (the newly coming docs are mostly about the user API part). I guess I will put this doc in Documentation/staging :) and I will finish the draft ASAP. Thanks Lin
On Wed, 2 Aug 2023 08:26:06 +0800 (GMT+08:00) Lin Ma wrote: > This is true. Actually, those check missing codes are mostly old codes and > modern netlink consumers will choose the general netlink interface which > can automatically get attributes description from YAML and never need to > do things like *manual parsing* anymore. > > However, according to my practice in auditing the code, I found there are > some general netlink interface users confront other issues like choosing > GENL_DONT_VALIDATE_STRICT without thinking or forgetting add a new > nla_policy when introducing new attributes. > > To this end, I'm currently writing a simple documentation about Netlink > interface best practices for the kernel developer (the newly coming docs > are mostly about the user API part). Keep in mind that even most of the genetlink stuff is pretty old at this stage. ethtool is probably the first reasonably modern family. But do send docs, we'll review and go from there :)
diff --git a/include/net/netlink.h b/include/net/netlink.h index b12cd957abb4..d825a5672161 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -229,6 +229,9 @@ enum nla_policy_validation { * nested header (or empty); len field is used if * nested_policy is also used, for the max attr * number in the nested policy. + * For NLA_NESTED whose nested nlattr is not necessary, + * the len field will indicate the exptected length of + * them for checking. * NLA_U8, NLA_U16, * NLA_U32, NLA_U64, * NLA_S8, NLA_S16, @@ -372,6 +375,9 @@ struct nla_policy { _NLA_POLICY_NESTED(ARRAY_SIZE(policy) - 1, policy) #define NLA_POLICY_NESTED_ARRAY(policy) \ _NLA_POLICY_NESTED_ARRAY(ARRAY_SIZE(policy) - 1, policy) +/* not care about the nested attributes, just do length check */ +#define NLA_POLICY_NESTED_NO_TYPE(length) \ + _NLA_POLICY_NESTED(length, NULL) #define NLA_POLICY_BITFIELD32(valid) \ { .type = NLA_BITFIELD32, .bitfield32_valid = valid } diff --git a/lib/nlattr.c b/lib/nlattr.c index 489e15bde5c1..29a412b41d28 100644 --- a/lib/nlattr.c +++ b/lib/nlattr.c @@ -488,6 +488,18 @@ static int validate_nla(const struct nlattr *nla, int maxtype, */ return err; } + } else if (pt->len) { + /* length set without nested_policy, the len field will + * be used to check those nested attributes here, + * we will not do parse here but just validation as the + * consumers will do manual parsing. + */ + const struct nlattr *nla_nested; + int rem; + + nla_for_each_attr(nla_nested, nla_data(nla), nla_len(nla), rem) + if (nla_len(nla_nested) < pt->len) + return -EINVAL; } break; case NLA_NESTED_ARRAY: