Message ID | 20230725023330.422856-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:9010:0:b0:3e4:2afc:c1 with SMTP id l16csp2190890vqg; Mon, 24 Jul 2023 19:44:03 -0700 (PDT) X-Google-Smtp-Source: APBJJlEepr5zxJaQzzz1DhWfLvan9I76itOtHlQeMYWX+JO2jFSegWqzGqFA+phk/dPyAjdoHLc1 X-Received: by 2002:aa7:d882:0:b0:522:1eab:e466 with SMTP id u2-20020aa7d882000000b005221eabe466mr5736578edq.28.1690253043283; Mon, 24 Jul 2023 19:44:03 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690253043; cv=none; d=google.com; s=arc-20160816; b=H3f7+t3v493Mq7QpuzgZ6J/m51tTTBWinybgo0aCfwSws+k4Cpsfiq/CarC1rW76Ec M1XHeppM/v21L3zLStEzo9diSkompCIVeR3UEkTyrVUQNt2Yd1V9c0U0bUcwIb8arGWo x8JOC66YY236NPo7oaxlgSe3Qzr1oZebZrilvb58QBrmokDm6pYjZEtuBVSfDYDu547n +9MA0BEP5OEa/fSwYgqHjOuwDJNmwbXciTcxdeODCnPA5Ft1tnInpsce10DrpH6mt48S ZPdlLmwiz+Gf5sMpJ97mn3T/plnWJu2W2nlDCDY5eBaWJ2gzQWbl2rWjHh2Ooc2ql9Of lXog== 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=QMwpbcAVGSl94S+vgTh6RhRmWBkkaAeXf1fD/eiHClE=; fh=b/cEWWmi+CD6WKka+TJy0VGgpKdMR+rkHocZS/mV9+c=; b=DKRlqSef8m8rzJ08uvLWl5Ekt9rGbtTHwlk0THDadHHwtf95dRe92kKWzVBzr872lp ezwGcDKDC23KhLLxW3rtmo48uP2NkShDPvZZ0MJWoJHRTW9wZe00BgB/44meWwc1RB1J WErXWunHZI3ECviLubfkb7At9N+XWga6NiAUaaQrIwBTBQbfGS+KZbovX5kTiGNgc+Hy Awnzy5JGghU3YfTf+ZF9BVwrFx2trSoiTV7vwu7C6mIzatPyhhZsa4e7yxRvTBVs3nR3 gLH6YsqIjFrB6I9BimvLfBbRKS68WusONQEXRT0KOApN3GtGUOhveC8svF/5fUI8dP9c tzmA== 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 w10-20020a056402070a00b00522414b4882si777324edx.187.2023.07.24.19.43.39; Mon, 24 Jul 2023 19:44:02 -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 S229977AbjGYCd4 (ORCPT <rfc822;kautuk.consul.80@gmail.com> + 99 others); Mon, 24 Jul 2023 22:33:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33816 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230216AbjGYCdy (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 24 Jul 2023 22:33:54 -0400 Received: from zg8tmtyylji0my4xnjqumte4.icoremail.net (zg8tmtyylji0my4xnjqumte4.icoremail.net [162.243.164.118]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 29C711737; Mon, 24 Jul 2023 19:33:50 -0700 (PDT) Received: from localhost.localdomain (unknown [36.24.99.225]) by mail-app3 (Coremail) with SMTP id cC_KCgBXPot7NL9k9rWkCw--.5856S4; Tue, 25 Jul 2023 10:33:31 +0800 (CST) From: Lin Ma <linma@zju.edu.cn> To: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, ast@kernel.org, martin.lau@kernel.org, yhs@fb.com, void@manifault.com, andrii@kernel.org, houtao1@huawei.com, inwardvessel@gmail.com, linma@zju.edu.cn, kuniyu@amazon.com, songliubraving@fb.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org Subject: [PATCH v2] bpf: Add length check for SK_DIAG_BPF_STORAGE_REQ_MAP_FD parsing Date: Tue, 25 Jul 2023 10:33:30 +0800 Message-Id: <20230725023330.422856-1-linma@zju.edu.cn> X-Mailer: git-send-email 2.17.1 X-CM-TRANSID: cC_KCgBXPot7NL9k9rWkCw--.5856S4 X-Coremail-Antispam: 1UD129KBjvJXoW7uF4kXFy8JryDuw13KFykZrb_yoW8GFy7pa s7Gr9xKr9rJrWfCFn7Jrsxua4UCw4UXFy3WFs8Zw4fZw4qqa43Gry3GF1Fqw15ArWrW3Wa yr1YgFy3ur9rZaDanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvG14x267AKxVW5JVWrJwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26w1j6s0DM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26F4U JVW0owA2z4x0Y4vEx4A2jsIE14v26rxl6s0DM28EF7xvwVC2z280aVCY1x0267AKxVW0oV Cq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0 I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r 4UM4x0Y48IcxkI7VAKI48JM4x0x7Aq67IIx4CEVc8vx2IErcIFxwACI402YVCY1x02628v n2kIc2xKxwCY02Avz4vE14v_Gr1l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr 0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY 17CE14v26r4a6rW5MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcV C0I7IYx2IY6xkF7I0E14v26r4j6F4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY 6I8E87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r4j6r4UJbIYCTnIWIevJa 73UjIFyTuYvjfUOTmhUUUUU X-CM-SenderInfo: qtrwiiyqvtljo62m3hxhgxhubq/ X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_MSPIKE_H2, 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: 1772199070401548230 X-GMAIL-MSGID: 1772358775383406021 |
Series |
[v2] bpf: Add length check for SK_DIAG_BPF_STORAGE_REQ_MAP_FD parsing
|
|
Commit Message
Lin Ma
July 25, 2023, 2:33 a.m. UTC
The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc
does not check the length of the nested attribute. This can lead to an
out-of-attribute read and allow a malformed nlattr (e.g., length 0) to
be viewed as a 4 byte integer.
This patch adds an additional check when the nlattr is getting counted.
This makes sure the latter nla_get_u32 can access the attributes with
the correct length.
Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
V1 -> V2: moves the check to the counting loop as Jakub suggested,
alters the commit message accordingly.
net/core/bpf_sk_storage.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On Tue, Jul 25, 2023 at 10:33:30AM +0800, Lin Ma wrote: > The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc > does not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as a 4 byte integer. > > This patch adds an additional check when the nlattr is getting counted. > This makes sure the latter nla_get_u32 can access the attributes with > the correct length. > > Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Lin Ma <linma@zju.edu.cn> > --- > V1 -> V2: moves the check to the counting loop as Jakub suggested, > alters the commit message accordingly. > > net/core/bpf_sk_storage.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c > index d4172534dfa8..cca7594be92e 100644 > --- a/net/core/bpf_sk_storage.c > +++ b/net/core/bpf_sk_storage.c > @@ -496,8 +496,11 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs) > return ERR_PTR(-EPERM); > > nla_for_each_nested(nla, nla_stgs, rem) { > - if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) > + if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) { > + if (nla_len(nla) != sizeof(u32)) Jakub, it seems like Lin adds this check to all nla_for_each_nested() loops. IMHO, the better change will be to change nla_for_each_nested() skip empty/not valid NLAs. Thanks > + return ERR_PTR(-EINVAL); > nr_maps++; > + } > } > > diag = kzalloc(struct_size(diag, maps, nr_maps), GFP_KERNEL); > -- > 2.17.1 > >
Hello Leon, > > Jakub, it seems like Lin adds this check to all nla_for_each_nested() loops. > IMHO, the better change will be to change nla_for_each_nested() skip empty/not valid NLAs. > > Thanks I guess you just get these fixes misunderstood. I do not add the nla_len check to **all nla_for_each_nested** :(. I only add checks to those who do not access the attributes without verifying the length, which is buggy. The others, either do a similar nla_len check already or just do nla_validate somewhere else. That is to say, they **validate** the relevant attributes. In short, nla_for_each_nested is just a loop macro that iterates the nlattrs, like nla_for_each macro. It is weird for them to do nlattr validation as there could have already been a call to nla_validate to ensure those attributes are correct. That is, for those who do not, a simple nla_len check is the simplest and most efficient choice. Regards Lin
On Tue, Jul 25, 2023 at 01:24:38PM +0800, Lin Ma wrote: > Hello Leon, > > > > > Jakub, it seems like Lin adds this check to all nla_for_each_nested() loops. > > IMHO, the better change will be to change nla_for_each_nested() skip empty/not valid NLAs. > > > > Thanks > > I guess you just get these fixes misunderstood. I do not add the nla_len check > to **all nla_for_each_nested** :(. I only add checks to those who do not access > the attributes without verifying the length, which is buggy. > > The others, either do a similar nla_len check already or just do nla_validate > somewhere else. That is to say, they **validate** the relevant attributes. > > In short, nla_for_each_nested is just a loop macro that iterates the nlattrs, > like nla_for_each macro. It is weird for them to do nlattr validation as there > could have already been a call to nla_validate to ensure those attributes are > correct. That is, for those who do not, a simple nla_len check is the simplest > and most efficient choice. My concern is related to maintainability in long run. Your check adds another layer of cabal knowledge which will be copied/pasted in other places. Thanks > > Regards > Lin
Hello Leon, > > My concern is related to maintainability in long run. Your check adds > another layer of cabal knowledge which will be copied/pasted in other > places. > > Thanks > Yeah, I guess you are right. I guess I should not just *fix* this issue but also think of the maintainability. The very first idea pop into my mind is to complete the necessary nla_policy hence the invalid nlattrs could be rejected at the very first place. Will spend more time on this. Regards Lin
On Tue, 25 Jul 2023 10:33:30 +0800 Lin Ma wrote: > The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc > does not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as a 4 byte integer. > > This patch adds an additional check when the nlattr is getting counted. > This makes sure the latter nla_get_u32 can access the attributes with > the correct length. > > Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Lin Ma <linma@zju.edu.cn> Reviewed-by: Jakub Kicinski <kuba@kernel.org> Those who parse manually must do checks manually. It is what it is.
On Tue, 2023-07-25 at 10:33 +0800, Lin Ma wrote: > The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc > does not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as a 4 byte integer. > > This patch adds an additional check when the nlattr is getting counted. > This makes sure the latter nla_get_u32 can access the attributes with > the correct length. > > Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Lin Ma <linma@zju.edu.cn> I guess this should go via the ebpf tree, right? Setting the delegate accordingly. Please correct me if I'm wrong. Thanks! /P
On 7/27/23 12:34 AM, Paolo Abeni wrote: > On Tue, 2023-07-25 at 10:33 +0800, Lin Ma wrote: >> The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc >> does not check the length of the nested attribute. This can lead to an >> out-of-attribute read and allow a malformed nlattr (e.g., length 0) to >> be viewed as a 4 byte integer. >> >> This patch adds an additional check when the nlattr is getting counted. >> This makes sure the latter nla_get_u32 can access the attributes with >> the correct length. >> >> Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") >> Suggested-by: Jakub Kicinski <kuba@kernel.org> >> Signed-off-by: Lin Ma <linma@zju.edu.cn> > > I guess this should go via the ebpf tree, right? Setting the delegate > accordingly. Already applied to the bpf tree. Thanks. pw-bot seems not doing auto-reply for the bpf tree.
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c index d4172534dfa8..cca7594be92e 100644 --- a/net/core/bpf_sk_storage.c +++ b/net/core/bpf_sk_storage.c @@ -496,8 +496,11 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs) return ERR_PTR(-EPERM); nla_for_each_nested(nla, nla_stgs, rem) { - if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) + if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) { + if (nla_len(nla) != sizeof(u32)) + return ERR_PTR(-EINVAL); nr_maps++; + } } diag = kzalloc(struct_size(diag, maps, nr_maps), GFP_KERNEL);