Message ID | 20231130200058.work.520-kees@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp649568vqy; Thu, 30 Nov 2023 12:02:34 -0800 (PST) X-Google-Smtp-Source: AGHT+IFlKFNQw25/hca5EQC+HmFxfoJPrhu7qanlCG4aMDdrczUWF8crFTFgPt+Tf0biIExOpVwN X-Received: by 2002:a17:902:e752:b0:1cf:b786:f110 with SMTP id p18-20020a170902e75200b001cfb786f110mr23258494plf.32.1701374553797; Thu, 30 Nov 2023 12:02:33 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701374553; cv=none; d=google.com; s=arc-20160816; b=EEH6H89KAGCdr2Y7cnBkd7bKFYhEbyjmS8aBmXEzay0R0kZI3Dw+fXtO+3nwshquGk EY0DtEStVytLtcPhaJ684xjT2o9NQLp6pb1NlMB0NioqGAwJd1UM1FE3pRhqXkWKxiiA KqIBXIWTEMN+rJD0G9Fif5n4gRsaK0zOTbPruOokvzBosWhQpDVANmfki1gyq3pxlPBg c+BEwmRl9qAQNQEj06dQ6uFkc0u6FjjedjGt9O+R4p+kLDFmV9uzG2k71xxMfJpluV2A fL7p6S/4l3x4zPxrZeUa13DgpLWz0WShcrnM3yfjsHxN5PUBsP3Aj/joT/VPHPuB8ypD awoA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=DjLamxZyQfbyptpIvpzFIM9HoGVb6+OmTO9thE+0l0A=; fh=LwoNQDDfBETRdPEeloq/UXMkPZiP7Uy41sc4YtS7ouI=; b=Omr1OHDsVHUJC9CCFpW36ZRmIJh3NcjRBLNDST7cpctwhNjhJFHNSbDMAGpFrRFqeG XdUpgSEnrQbuUsBVUfvPtIEwG4Q7CYukiwClrB2DLy4TcgV5PoQKwA9dbbH13BC329N5 bWD98gmdq0VPBrFMEQ+eVhIiDUlO0lpBgmWJ9ZRCmosOdqpIOP5B+EFX49cR5uVWrFMH MfnPn5yOGTyIKSYDzLh/Z+mYR/biWRNGAeaTVOzECqNb3SLo02DqbSq3BgQGen9gchPG pWHhE2adCWrHOxFhEbSLIaQU3MJtHyLLrREd4F5yuaVlq8WLeA4e6RS1nGWBzvve4MiQ 5MAw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=GGyJ41Gi; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id t67-20020a632d46000000b005b930e0b600si1877961pgt.820.2023.11.30.12.02.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Nov 2023 12:02:33 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=GGyJ41Gi; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 7B035807FCFA; Thu, 30 Nov 2023 12:01:17 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232286AbjK3UA7 (ORCPT <rfc822;ruipengqi7@gmail.com> + 99 others); Thu, 30 Nov 2023 15:00:59 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43696 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229623AbjK3UA6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 30 Nov 2023 15:00:58 -0500 Received: from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com [IPv6:2607:f8b0:4864:20::1030]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7536110F8 for <linux-kernel@vger.kernel.org>; Thu, 30 Nov 2023 12:01:04 -0800 (PST) Received: by mail-pj1-x1030.google.com with SMTP id 98e67ed59e1d1-286406ae852so725703a91.0 for <linux-kernel@vger.kernel.org>; Thu, 30 Nov 2023 12:01:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1701374464; x=1701979264; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=DjLamxZyQfbyptpIvpzFIM9HoGVb6+OmTO9thE+0l0A=; b=GGyJ41GinrkCDijL/CNfx8rZzdU+bWwf+Sretpgifqmsok/ABKqMO18zKMQK1XPOLv GkFrv6Eq7jtCBnl6+iPQCJQNQCbeRV7WwdarFUCZV3qBmHrrO5Stq5YMAk0DeDMnv3AA tV+ja7xTeosbnInimbMe/Rfe4j/CPfl07T2H0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701374464; x=1701979264; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=DjLamxZyQfbyptpIvpzFIM9HoGVb6+OmTO9thE+0l0A=; b=Qy4TDk0y0+UDN7lMz2RoTrL4PvUCYZvYSQE1V6s+BFA0p1RDZIkbDxrG90QD/yjjox z+H1Zf8o2lAFtXHFdHJ4MVrVJ27h7zE8KR/Ks/ajDcD3u0PXWXqV34/YrTBs5W7KNrVR /4sqids0rPCj6f17DaqWh+B7e4Njwp2n7yfrCM4zKyl0uus86/dFoe08MoDBlo7gyaVy HO4CclOZmSyIlpcFnniCehvHWLx9/lo7qfEvaNr+9SngY/QKZixFT54dXJRxuG2m3+ql anEIZ3Y3S6up3nxNY9rmhHoBdDEw2edMz6VXKiw7z+mk8/Es6r/XsSjDf23ml+hXBLFW /yRQ== X-Gm-Message-State: AOJu0Yz4Xy3P0dTLW0quiTX0n57voGTxvLfHvS1pe+u9XlqYXFowM3yT UzEM3xZBIsYQuBF3N0ggMsMVD9AbEXWO2LZaljg= X-Received: by 2002:a17:90b:3b86:b0:285:a179:7174 with SMTP id pc6-20020a17090b3b8600b00285a1797174mr22305244pjb.29.1701374463664; Thu, 30 Nov 2023 12:01:03 -0800 (PST) Received: from www.outflux.net (198-0-35-241-static.hfc.comcastbusiness.net. [198.0.35.241]) by smtp.gmail.com with ESMTPSA id gg16-20020a17090b0a1000b002858ac5e401sm3687765pjb.45.2023.11.30.12.01.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Nov 2023 12:01:03 -0800 (PST) From: Kees Cook <keescook@chromium.org> To: Jakub Kicinski <kuba@kernel.org> Cc: Kees Cook <keescook@chromium.org>, kernel test robot <lkp@intel.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, Johannes Berg <johannes@sipsolutions.net>, Jeff Johnson <quic_jjohnson@quicinc.com>, Michael Walle <mwalle@kernel.org>, Max Schulze <max.schulze@online.de>, netdev@vger.kernel.org, linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: [PATCH] netlink: Return unsigned value for nla_len() Date: Thu, 30 Nov 2023 12:01:01 -0800 Message-Id: <20231130200058.work.520-kees@kernel.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2452; i=keescook@chromium.org; h=from:subject:message-id; bh=penjs9WkGWzsqXkZBizH1HvU0T+dKnNfeIaWwvPQsRs=; b=owEBbQKS/ZANAwAKAYly9N/cbcAmAcsmYgBlaOn9cgfbrAAjas3hx+hd183f+KUlnaqvvTcKU c2eIz0CGr+JAjMEAAEKAB0WIQSlw/aPIp3WD3I+bhOJcvTf3G3AJgUCZWjp/QAKCRCJcvTf3G3A JmIKD/9fiJNBEEe9sj4peZsNPvhzqGeV+ixJOnS3btR7fRWXAR9QBI7p3472rOKKDSEWZRdQTRA B5AlP8Marc+gBPUBdlUI2yQuf7iLVJ01rwNk7MwwIm0p8877bB9Ge135CpOadBdbi6N52wYlslO fbY82GwnMjEL9QLeGY+bnVljthqsh0u4kzcj5lvIekba/DjxD02UIl6gDiGOjBKEdXYXH0W5+7B uW1xm50ZqStiyMkZZNdBsN/x+4w0StkgnevqMlHan+qTAGKPSQjfgBwnfOT3z/FVMnCGQyCtxYn 9KfatOF3VjZl46DHRNfQge4XVW5DOmOROeWhrNkQO37hOF0WQxOkonKxggOmynztvZXky5GdJJk tiLIaHyOqJWjxqwHX3USmrjr8WoFXqUGLZUkIj/opc+VpAHdsRN4ZwFLeXcPBLTiC+NWdxXKBo8 O520MI1VJ1DXdtevqUR5SSfNbJcdcPnvAYoi6HLgonCzGcTextAdQBvRSp21GJ/2V3HMZVkKlUw ljYYLzEDitegFiqrm8+9uwtU+sELR7aZ5uaeM0Yi8Co6ml4VYGgxOHKANC8kTvqTGL03DbLrCfV dh6a2w5v9R7rWMFPujOVF3bg8C82w7O6z1FrfxDu3Z/+4EnrtLPo0zdTUkrQe6/6HW1xnnx89m3 N9xuByR CH8DD7Tw== X-Developer-Key: i=keescook@chromium.org; a=openpgp; fpr=A5C3F68F229DD60F723E6E138972F4DFDC6DC026 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 30 Nov 2023 12:01:17 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784020523991070218 X-GMAIL-MSGID: 1784020523991070218 |
Series |
netlink: Return unsigned value for nla_len()
|
|
Commit Message
Kees Cook
Nov. 30, 2023, 8:01 p.m. UTC
The return value from nla_len() is never expected to be negative, and can
never be more than struct nlattr::nla_len (a u16). Adjust the prototype
on the function, and explicitly bounds check the subtraction. This will
let GCC's value range optimization passes know that the return can never
be negative, and can never be larger than u16. As recently discussed[1],
this silences the following warning in GCC 12+:
net/wireless/nl80211.c: In function 'nl80211_set_cqm_rssi.isra':
net/wireless/nl80211.c:12892:17: warning: 'memcpy' specified bound 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
12892 | memcpy(cqm_config->rssi_thresholds, thresholds,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
12893 | flex_array_size(cqm_config, rssi_thresholds,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
12894 | n_thresholds));
| ~~~~~~~~~~~~~~
This has the additional benefit of being defensive in the face of nlattr
corruption or logic errors (i.e. nla_len being set smaller than
NLA_HDRLEN).
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311090752.hWcJWAHL-lkp@intel.com/ [1]
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Jeff Johnson <quic_jjohnson@quicinc.com>
Cc: Michael Walle <mwalle@kernel.org>
Cc: Max Schulze <max.schulze@online.de>
Cc: netdev@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/net/netlink.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
On 11/30/23 14:01, Kees Cook wrote: > The return value from nla_len() is never expected to be negative, and can > never be more than struct nlattr::nla_len (a u16). Adjust the prototype > on the function, and explicitly bounds check the subtraction. This will > let GCC's value range optimization passes know that the return can never > be negative, and can never be larger than u16. As recently discussed[1], > this silences the following warning in GCC 12+: > > net/wireless/nl80211.c: In function 'nl80211_set_cqm_rssi.isra': > net/wireless/nl80211.c:12892:17: warning: 'memcpy' specified bound 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=] > 12892 | memcpy(cqm_config->rssi_thresholds, thresholds, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 12893 | flex_array_size(cqm_config, rssi_thresholds, > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 12894 | n_thresholds)); > | ~~~~~~~~~~~~~~ > > This has the additional benefit of being defensive in the face of nlattr > corruption or logic errors (i.e. nla_len being set smaller than > NLA_HDRLEN). > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202311090752.hWcJWAHL-lkp@intel.com/ [1] > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Johannes Berg <johannes@sipsolutions.net> > Cc: Jeff Johnson <quic_jjohnson@quicinc.com> > Cc: Michael Walle <mwalle@kernel.org> > Cc: Max Schulze <max.schulze@online.de> > Cc: netdev@vger.kernel.org > Cc: linux-wireless@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Looks good to me. Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks -- Gustavo > --- > include/net/netlink.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/net/netlink.h b/include/net/netlink.h > index 167b91348e57..c59679524705 100644 > --- a/include/net/netlink.h > +++ b/include/net/netlink.h > @@ -1214,9 +1214,9 @@ static inline void *nla_data(const struct nlattr *nla) > * nla_len - length of payload > * @nla: netlink attribute > */ > -static inline int nla_len(const struct nlattr *nla) > +static inline u16 nla_len(const struct nlattr *nla) > { > - return nla->nla_len - NLA_HDRLEN; > + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0; > } > > /**
On Thu, 30 Nov 2023 12:01:01 -0800 Kees Cook wrote: > This has the additional benefit of being defensive in the face of nlattr > corruption or logic errors (i.e. nla_len being set smaller than > NLA_HDRLEN). As Johannes predicted I'd rather not :( The callers should put the nlattr thru nla_ok() during validation (nla_validate()), or walking (nla_for_each_* call nla_ok()). > -static inline int nla_len(const struct nlattr *nla) > +static inline u16 nla_len(const struct nlattr *nla) > { > - return nla->nla_len - NLA_HDRLEN; > + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0; > } Note the the NLA_HDRLEN is the length of struct nlattr. I mean of the @nla object that gets passed in as argument here. So accepting that nla->nla_len may be < NLA_HDRLEN means that we are okay with dereferencing a truncated object... We can consider making the return unsinged without the condition maybe?
On Thu, 2023-11-30 at 17:25 -0800, Jakub Kicinski wrote: > On Thu, 30 Nov 2023 12:01:01 -0800 Kees Cook wrote: > > This has the additional benefit of being defensive in the face of nlattr > > corruption or logic errors (i.e. nla_len being set smaller than > > NLA_HDRLEN). > > As Johannes predicted I'd rather not :( :) > The callers should put the nlattr thru nla_ok() during validation > (nla_validate()), or walking (nla_for_each_* call nla_ok()). Which we do, since we have just normal input validation on generic netlink. Actually nla_validate() only does it via walking either ;-) The thing is that's something the compiler can't really see, it happens out-of-line in completely different code (generic netlink) before you even get into nl80211. > > -static inline int nla_len(const struct nlattr *nla) > > +static inline u16 nla_len(const struct nlattr *nla) > > { > > - return nla->nla_len - NLA_HDRLEN; > > + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0; > > } > > Note the the NLA_HDRLEN is the length of struct nlattr. > I mean of the @nla object that gets passed in as argument here. > So accepting that nla->nla_len may be < NLA_HDRLEN means > that we are okay with dereferencing a truncated object... > > We can consider making the return unsinged without the condition maybe? That seems problematic too though - better for an (unvalidated) attribute with a bad size to actually show up with a negative payload length rather than an underflow to a really big size. Anyway I really don't mind the workaround in nl80211 (which was to make the variables holding this unsigned), since we *do* know that we validated there, that's not an issue wrt. the length. johannes
On Thu, Nov 30, 2023 at 05:25:20PM -0800, Jakub Kicinski wrote: > On Thu, 30 Nov 2023 12:01:01 -0800 Kees Cook wrote: > > This has the additional benefit of being defensive in the face of nlattr > > corruption or logic errors (i.e. nla_len being set smaller than > > NLA_HDRLEN). > > As Johannes predicted I'd rather not :( > > The callers should put the nlattr thru nla_ok() during validation > (nla_validate()), or walking (nla_for_each_* call nla_ok()). > > > -static inline int nla_len(const struct nlattr *nla) > > +static inline u16 nla_len(const struct nlattr *nla) > > { > > - return nla->nla_len - NLA_HDRLEN; > > + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0; > > } > > Note the the NLA_HDRLEN is the length of struct nlattr. > I mean of the @nla object that gets passed in as argument here. > So accepting that nla->nla_len may be < NLA_HDRLEN means > that we are okay with dereferencing a truncated object... > > We can consider making the return unsinged without the condition maybe? Yes, if we did it without the check, it'd do "less" damage on wrap-around. (i.e. off by U16_MAX instead off by INT_MAX). But I'd like to understand: what's the harm in adding the clamp? The changes to the assembly are tiny: https://godbolt.org/z/Ecvbzn1a1 i.e. a likely dropped-from-the-pipeline xor and a "free" cmov (checking the bit from the subtraction). I don't think it could even get measured in real-world cycle counts. This is much like the refcount_t work: checking for the overflow condition has almost 0 overhead. (It looks like I should use __builtin_sub_overflow() to correctly hint GCC, but Clang gets it right without such hinting. Also I changed NLA_HDRLEN to u16 to get the best result, which suggests there might be larger savings throughout the code base just from that change...)
On Fri, 1 Dec 2023 10:17:02 -0800 Kees Cook wrote: > > > -static inline int nla_len(const struct nlattr *nla) > > > +static inline u16 nla_len(const struct nlattr *nla) > > > { > > > - return nla->nla_len - NLA_HDRLEN; > > > + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0; > > > } > > > > Note the the NLA_HDRLEN is the length of struct nlattr. > > I mean of the @nla object that gets passed in as argument here. > > So accepting that nla->nla_len may be < NLA_HDRLEN means > > that we are okay with dereferencing a truncated object... > > > > We can consider making the return unsinged without the condition maybe? > > Yes, if we did it without the check, it'd do "less" damage on > wrap-around. (i.e. off by U16_MAX instead off by INT_MAX). > > But I'd like to understand: what's the harm in adding the clamp? The > changes to the assembly are tiny: > https://godbolt.org/z/Ecvbzn1a1 Hm, I wonder if my explanation was unclear or you disagree.. This is the structure: struct nlattr { __u16 nla_len; // attr len, incl. this header __u16 nla_type; }; and (removing no-op wrappers): #define NLA_HDRLEN sizeof(struct nlattr) So going back to the code: return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN... We are reading nla->nla_len, which is the first 2 bytes of the structure. And then we check if the structure is... there? If we don't trust that struct nlattr which gets passed here is at least NLA_HDRLEN (4B) then why do we think it's safe to read nla_len (the first 2B of it)? That's why I was pointing at nla_ok(). nla_ok() takes the size of the buffer / message as an arg, so that it can also check if looking at nla_len itself is not going to be an OOB access. 99% of netlink buffers we parse come from user space. So it's not like someone could have mis-initialized the nla_len in the kernel and being graceful is helpful. The extra conditional is just a minor thing. The major thing is that unless I'm missing something the check makes me go 🤨️
On Fri, Dec 01, 2023 at 10:45:05AM -0800, Jakub Kicinski wrote: > On Fri, 1 Dec 2023 10:17:02 -0800 Kees Cook wrote: > > > > -static inline int nla_len(const struct nlattr *nla) > > > > +static inline u16 nla_len(const struct nlattr *nla) > > > > { > > > > - return nla->nla_len - NLA_HDRLEN; > > > > + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0; > > > > } > > > > > > Note the the NLA_HDRLEN is the length of struct nlattr. > > > I mean of the @nla object that gets passed in as argument here. > > > So accepting that nla->nla_len may be < NLA_HDRLEN means > > > that we are okay with dereferencing a truncated object... > > > > > > We can consider making the return unsinged without the condition maybe? > > > > Yes, if we did it without the check, it'd do "less" damage on > > wrap-around. (i.e. off by U16_MAX instead off by INT_MAX). > > > > But I'd like to understand: what's the harm in adding the clamp? The > > changes to the assembly are tiny: > > https://godbolt.org/z/Ecvbzn1a1 > > Hm, I wonder if my explanation was unclear or you disagree.. > > This is the structure: > > struct nlattr { > __u16 nla_len; // attr len, incl. this header > __u16 nla_type; > }; > > and (removing no-op wrappers): > > #define NLA_HDRLEN sizeof(struct nlattr) > > So going back to the code: > > return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN... > > We are reading nla->nla_len, which is the first 2 bytes of the structure. > And then we check if the structure is... there? I'm not debating whether it's there or not -- I'm saying the _contents_ of "nlattr::nla_len", in the face of corruption or lack of initialization, may be less than NLA_HDRLEN. (There's a lot of "but that's can't happen" that _does_ happen in the kernel, so I'm extra paranoid.) > If we don't trust that struct nlattr which gets passed here is at least > NLA_HDRLEN (4B) then why do we think it's safe to read nla_len (the > first 2B of it)? Type confusion (usually due to Use-after-Free flaws) means that a memory region is valid (i.e. good pointer), but that the contents might have gotten changed through other means. (To see examples of this with struct msg_msg, see: https://syst3mfailure.io/wall-of-perdition/) (On a related note, why does nla_len start at 4 instead of 0? i.e. why does it include the size of nlattr? That seems redundant based on the same logic you're using here.) > That's why I was pointing at nla_ok(). nla_ok() takes the size of the > buffer / message as an arg, so that it can also check if looking at > nla_len itself is not going to be an OOB access. 99% of netlink buffers > we parse come from user space. So it's not like someone could have > mis-initialized the nla_len in the kernel and being graceful is helpful. > > The extra conditional is just a minor thing. The major thing is that > unless I'm missing something the check makes me go 🤨️ My concern is that there are 562 callers of nla_len(): $ git grep '\bnla_len(\b' | wc -l 562 We have no way to be certain that all callers follow a successful nla_ok() call. Regardless, just moving from "int" to "u16" solves a bunch of value range tracking pain that GCC appears to get upset about, so if you really don't want the (tiny) sanity check, I can just send the u16 change. -Kees
On Fri, 1 Dec 2023 20:39:44 -0800 Kees Cook wrote: > > We are reading nla->nla_len, which is the first 2 bytes of the structure. > > And then we check if the structure is... there? > > I'm not debating whether it's there or not -- I'm saying the _contents_ of > "nlattr::nla_len", in the face of corruption or lack of initialization, > may be less than NLA_HDRLEN. (There's a lot of "but that's can't happen" > that _does_ happen in the kernel, so I'm extra paranoid.) nlattr is not an object someone has allocated. It's a header of a TLV in a byte stream of nested TLVs which comes from user space. If the attr did not go thru nla_ok() or some other careful validation we're toast regardless. > > If we don't trust that struct nlattr which gets passed here is at least > > NLA_HDRLEN (4B) then why do we think it's safe to read nla_len (the > > first 2B of it)? > > Type confusion (usually due to Use-after-Free flaws) means that a memory > region is valid (i.e. good pointer), but that the contents might have > gotten changed through other means. (To see examples of this with > struct msg_msg, see: https://syst3mfailure.io/wall-of-perdition/) A bit of a long read. > (On a related note, why does nla_len start at 4 instead of 0? i.e. why > does it include the size of nlattr? That seems redundant based on the > same logic you're using here.) Beats me.
diff --git a/include/net/netlink.h b/include/net/netlink.h index 167b91348e57..c59679524705 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -1214,9 +1214,9 @@ static inline void *nla_data(const struct nlattr *nla) * nla_len - length of payload * @nla: netlink attribute */ -static inline int nla_len(const struct nlattr *nla) +static inline u16 nla_len(const struct nlattr *nla) { - return nla->nla_len - NLA_HDRLEN; + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0; } /**