Message ID | 20231205091952.24754-1-pchelkin@ispras.ru |
---|---|
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 r17csp3306339vqy; Tue, 5 Dec 2023 01:20:59 -0800 (PST) X-Google-Smtp-Source: AGHT+IGtb1rw2oBANkiCfw1aHVj/SBBimtd48xwCFVOaqPQ6ybU8HEenSd8xDwgqV4rx3JHBFfm3 X-Received: by 2002:a17:903:230c:b0:1d0:6ffd:610e with SMTP id d12-20020a170903230c00b001d06ffd610emr1599412plh.48.1701768059121; Tue, 05 Dec 2023 01:20:59 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701768059; cv=none; d=google.com; s=arc-20160816; b=RFHmPbwNMoVm/cFfvtiHZsd5xPR+KB/TNgmxtRr6MvXhVDeI2yIuampIFQVdWXH5Ba NRM7/jopEpvpLPlL4s+5k/6066cGx8Eay6yXD/vBHUL2Qafh09ObwzuuQzP3iKxmhXGL g04FTRV5afBQbGV2ZRrfidI7vlebMAQI730HA7Hp+RtUIwIJNYgc2nz2OOieTcEzJafW ZsnlDg7mIqFobGYe8TcFH5i6FhczYx99/suaT01tDFmOPCE68sCgCaxCYX2lrvYLYjOQ vDq3ItiERK4YR9colC4IuNJA9F4BifNzAd1lBeVwqR91gnoO5ddtP3hPesna8xLNQgPG g3UQ== 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 :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature:dkim-filter; bh=Xkj0nuUUERVo3DmD0W5CbQml3tG2cDGWBXJRyDsrHq8=; fh=PiwcU1SnKSd+B7vTdLgfllmhsd7uYDomZCqbbHf7Qxw=; b=oNzsn/tccFOranl8rm2Z27bjMN0d9Xh+08o+3ju/wu+1Y4UDDJG/o0E5uzJHZ9KU/E mdTYGr5L2PAHcBoEZePz8pvuU5pGgneF7OoAVWwBcWvCZTs97/yGWpQri4HcMgYdN9te jkTWpvL/aaKAfbj99Yodm6SPrYTkBIbXOZwTlxbaF89sjKMr4QXuQlzaUdG9zOAdGQJr npiVqZBbulXpPjTk9jH1vsh+DFThtpZc0/W0UTHr5sRuh5AEZwBL/rdcuXbLvwQUF+3Q bQREWpcwnObyvChL/LvCoMaB2TxzOewHekzk/dhRu7DDaa/QRj4wp1knTMYY7e//wbXa HSwQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ispras.ru header.s=default header.b=OOs6rrNx; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=ispras.ru Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id b5-20020a170902d88500b001d0b9edced7si1823925plz.581.2023.12.05.01.20.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 01:20:59 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@ispras.ru header.s=default header.b=OOs6rrNx; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=ispras.ru Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id ADF64807BEED; Tue, 5 Dec 2023 01:20:26 -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 S234928AbjLEJUF (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Tue, 5 Dec 2023 04:20:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45162 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229584AbjLEJUE (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 5 Dec 2023 04:20:04 -0500 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0875483; Tue, 5 Dec 2023 01:20:09 -0800 (PST) Received: from localhost.ispras.ru (unknown [10.10.165.7]) by mail.ispras.ru (Postfix) with ESMTPSA id 3F8CD40F1DE9; Tue, 5 Dec 2023 09:20:04 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru 3F8CD40F1DE9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1701768007; bh=Xkj0nuUUERVo3DmD0W5CbQml3tG2cDGWBXJRyDsrHq8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=OOs6rrNxIXNTpiu+wwKGkoIxiCIgN0JjlOkTYcgNboYScKjXAtJUNbYxCx1uhxUV8 V+dSISTy/vUdFlBok1a3MZBkLoxXytP8ZzQhgiKfvPLttDjHjp4EYZz5IX8s9V5rXa 43jfNC40GiHt+8/BgkeP+wcGGsb2DrgnsNzJMeOs= From: Fedor Pchelkin <pchelkin@ispras.ru> To: Dominique Martinet <asmadeus@codewreck.org> Cc: Fedor Pchelkin <pchelkin@ispras.ru>, Latchesar Ionkov <lucho@ionkov.net>, Eric Van Hensbergen <ericvh@kernel.org>, Christian Schoenebeck <linux_oss@crudebyte.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, v9fs@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov <khoroshilov@ispras.ru>, lvc-project@linuxtesting.org Subject: [PATCH v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf Date: Tue, 5 Dec 2023 12:19:50 +0300 Message-ID: <20231205091952.24754-1-pchelkin@ispras.ru> X-Mailer: git-send-email 2.43.0 In-Reply-To: <ZW7oQ1KPWTbiGSzL@codewreck.org> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=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]); Tue, 05 Dec 2023 01:20:26 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784428474878386100 X-GMAIL-MSGID: 1784433144232263928 |
Series |
[v2] net: 9p: avoid freeing uninit memory in p9pdu_vreadf
|
|
Commit Message
Fedor Pchelkin
Dec. 5, 2023, 9:19 a.m. UTC
If an error occurs while processing an array of strings in p9pdu_vreadf
then uninitialized members of *wnames array are freed.
Fix this by iterating over only lower indices of the array. Also handle
possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case
fails.
Found by Linux Verification Center (linuxtesting.org).
Fixes: ace51c4dd2f9 ("9p: add new protocol support code")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
v2: I've missed that *wnames can also be left uninitialized. Please
ignore the patch v1. As an answer to Dominique's comment: my
organization marks this statement in all commits.
net/9p/protocol.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
Comments
Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 12:19:50PM +0300: > If an error occurs while processing an array of strings in p9pdu_vreadf > then uninitialized members of *wnames array are freed. > > Fix this by iterating over only lower indices of the array. Also handle > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > fails. > > Found by Linux Verification Center (linuxtesting.org). > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > --- > v2: I've missed that *wnames can also be left uninitialized. Please > ignore the patch v1. While I agree it's good to initialize it in general, how is that a problem here? Do we have users that'd ignore the return code and try to use *wnames? (The first initialization is required in case the first p9pdu_readf fails and *wnames had a non-null initial value, but the second is unrelated) I don't mind the change even if there isn't but let's add a word in the commit message. > As an answer to Dominique's comment: my organization marks this > statement in all commits. Fair enough, I think you'd get more internet points with a 'Reported-by' but I see plenty of such messages in old commits and this isn't something I want to argue about -- ok.
On 23/12/05 06:31PM, Dominique Martinet wrote: > Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 12:19:50PM +0300: > > If an error occurs while processing an array of strings in p9pdu_vreadf > > then uninitialized members of *wnames array are freed. > > > > Fix this by iterating over only lower indices of the array. Also handle > > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > > fails. > > > > Found by Linux Verification Center (linuxtesting.org). > > > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > > --- > > v2: I've missed that *wnames can also be left uninitialized. Please > > ignore the patch v1. > > While I agree it's good to initialize it in general, how is that a > problem here? Do we have users that'd ignore the return code and try to > use *wnames? > (The first initialization is required in case the first p9pdu_readf > fails and *wnames had a non-null initial value, but the second is > unrelated) > My initial concern was just about the statement you wrote in parenthesis. Case 'T' can be provided with non-null initial *wnames value, and if the first p9pdu_readf() call there fails then *wnames is invalidly freed in error handling path here: case 'T':{ [...] if (errcode) { if (*wnames) { int i; for (i = 0; i < *nwname; i++) kfree((*wnames)[i]); } kfree(*wnames); *wnames = NULL; } So the first initialization is required to prevent the described error. As for the second initialization (the one located after kfree(*wnames) in error handling path - it was there all the time), I think it's better not to touch it. I've just moved kfree and null-assignment under 'if (*wnames)' statement. The concern you mentioned is about any user that'd ignore the return code and try to use *wnames (so that the second initialization makes some sense). I can't see if there is any such user but, as said before, it's better not to touch that code. > I don't mind the change even if there isn't but let's add a word in the > commit message. > OK, will do in v3. > > As an answer to Dominique's comment: my organization marks this > > statement in all commits. > > Fair enough, I think you'd get more internet points with a 'Reported-by' > but I see plenty of such messages in old commits and this isn't > something I want to argue about -- ok. > > -- > Dominique Martinet | Asmadeus
On Tuesday, December 5, 2023 10:19:50 AM CET Fedor Pchelkin wrote: > If an error occurs while processing an array of strings in p9pdu_vreadf > then uninitialized members of *wnames array are freed. > > Fix this by iterating over only lower indices of the array. Also handle > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > fails. > > Found by Linux Verification Center (linuxtesting.org). > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > --- > v2: I've missed that *wnames can also be left uninitialized. Please > ignore the patch v1. As an answer to Dominique's comment: my > organization marks this statement in all commits. > > net/9p/protocol.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > index 4e3a2a1ffcb3..043b621f8b84 100644 > --- a/net/9p/protocol.c > +++ b/net/9p/protocol.c > @@ -393,6 +393,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > case 'T':{ > uint16_t *nwname = va_arg(ap, uint16_t *); > char ***wnames = va_arg(ap, char ***); > + int i; > + *wnames = NULL; Consider also initializing `int i = 0;` here. Because ... > > errcode = p9pdu_readf(pdu, proto_version, > "w", nwname); > @@ -406,8 +408,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > } > > if (!errcode) { > - int i; > - > for (i = 0; i < *nwname; i++) { ... this block that initializes `i` is conditional. I mean it does work right now as-is, because ... > errcode = > p9pdu_readf(pdu, > @@ -421,13 +421,11 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > if (errcode) { > if (*wnames) { > - int i; > - > - for (i = 0; i < *nwname; i++) > + while (--i >= 0) > kfree((*wnames)[i]); > + kfree(*wnames); > + *wnames = NULL; > } ... this is wrapped into `if (*wnames) {` and you initialized *wnames with NULL, but it just feels like a potential future trap somehow. Anyway, at least it looks like correct behaviour (ATM), so: Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> > - kfree(*wnames); > - *wnames = NULL; > } > } > break; >
Fedor Pchelkin wrote on Tue, Dec 05, 2023 at 03:15:43PM +0300: > As for the second initialization (the one located after kfree(*wnames) in > error handling path - it was there all the time), I think it's better not > to touch it. I've just moved kfree and null-assignment under > 'if (*wnames)' statement. Ah, I somehow missed this was just moved; that doesn't change anything but doesn't hurt either, sure. > The concern you mentioned is about any user that'd ignore the return code > and try to use *wnames (so that the second initialization makes some > sense). I can't see if there is any such user but, as said before, it's > better not to touch that code. Yes, it was here before, let's leave it in. > > I don't mind the change even if there isn't but let's add a word in the > > commit message. > > OK, will do in v3. I've queued to -next as is (with the i initialized as Christian pointed out), will update if you send a new one later. Thanks,
On 23/12/05 01:29PM, Christian Schoenebeck wrote: > On Tuesday, December 5, 2023 10:19:50 AM CET Fedor Pchelkin wrote: > > If an error occurs while processing an array of strings in p9pdu_vreadf > > then uninitialized members of *wnames array are freed. > > > > Fix this by iterating over only lower indices of the array. Also handle > > possible uninit *wnames usage if first p9pdu_readf() call inside 'T' case > > fails. > > > > Found by Linux Verification Center (linuxtesting.org). > > > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > > --- > > v2: I've missed that *wnames can also be left uninitialized. Please > > ignore the patch v1. As an answer to Dominique's comment: my > > organization marks this statement in all commits. > > > > net/9p/protocol.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > > index 4e3a2a1ffcb3..043b621f8b84 100644 > > --- a/net/9p/protocol.c > > +++ b/net/9p/protocol.c > > @@ -393,6 +393,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > case 'T':{ > > uint16_t *nwname = va_arg(ap, uint16_t *); > > char ***wnames = va_arg(ap, char ***); > > + int i; > > + *wnames = NULL; > > Consider also initializing `int i = 0;` here. Because ... > The hassle with indices in this code can be eliminated with using kcalloc() instead of kmalloc_array(). It would initialize all the members to zero and later we can use the fact that kfree() is a no-op for NULL args and iterate over all the elements - this trick is ubiquitous in kernel AFAIK. But when trying to do such kind of changes, I wonder whether it would impact performance (I'm not able to test this fully) or related issues as for some reason an unsafe kmalloc_array() was originally used. If you have no objections, then I'll better prepare a new patch with this in mind. That will make the code less prone to potential errors in future. > > > > errcode = p9pdu_readf(pdu, proto_version, > > "w", nwname); > > @@ -406,8 +408,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > } > > > > if (!errcode) { > > - int i; > > - > > for (i = 0; i < *nwname; i++) { > > ... this block that initializes `i` is conditional. I mean it does work right > now as-is, because ... > > > errcode = > > p9pdu_readf(pdu, > > @@ -421,13 +421,11 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > > > if (errcode) { > > if (*wnames) { > > - int i; > > - > > - for (i = 0; i < *nwname; i++) > > + while (--i >= 0) > > kfree((*wnames)[i]); > > + kfree(*wnames); > > + *wnames = NULL; > > } > > ... this is wrapped into `if (*wnames) {` and you initialized *wnames with > NULL, but it just feels like a potential future trap somehow. > > Anyway, at least it looks like correct behaviour (ATM), so: > > Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> > > > - kfree(*wnames); > > - *wnames = NULL; > > } > > } > > break; > > > >
diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 4e3a2a1ffcb3..043b621f8b84 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -393,6 +393,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, case 'T':{ uint16_t *nwname = va_arg(ap, uint16_t *); char ***wnames = va_arg(ap, char ***); + int i; + *wnames = NULL; errcode = p9pdu_readf(pdu, proto_version, "w", nwname); @@ -406,8 +408,6 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, } if (!errcode) { - int i; - for (i = 0; i < *nwname; i++) { errcode = p9pdu_readf(pdu, @@ -421,13 +421,11 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, if (errcode) { if (*wnames) { - int i; - - for (i = 0; i < *nwname; i++) + while (--i >= 0) kfree((*wnames)[i]); + kfree(*wnames); + *wnames = NULL; } - kfree(*wnames); - *wnames = NULL; } } break;