Message ID | 20231205180523.11318-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 r17csp3610263vqy; Tue, 5 Dec 2023 10:06:04 -0800 (PST) X-Google-Smtp-Source: AGHT+IHfNMTlChdhN+MgMKOrYxM0CGQcoQ0ZjNKIHxQlnYbc+lVx/zK306gAiNPLY8AH2VS4ttEr X-Received: by 2002:a05:6a20:7f99:b0:18d:64a:e125 with SMTP id d25-20020a056a207f9900b0018d064ae125mr8903944pzj.31.1701799563857; Tue, 05 Dec 2023 10:06:03 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701799563; cv=none; d=google.com; s=arc-20160816; b=u7ixu0Hghps2thXIRadalszsWmDvmXCQl2+4uhT0VUp7Zg7o7qjVJl1B6oPJPDnPp1 jaqMf9Ofsu7BMZ2W25TraKFZHQ3c8+2Gv9xxHYJBxeFPGw5o/bLDkcV0SI7qtXUM3M+D OftNssn9DPbEFVlX5zBMcrLifPcZcYV1+Pry5pJXPoKCVGgliHv1z2iHvhQda3+sd0Tu kuD6DMgu3uUWlOi+OpR2L1xm09Id/g1hXNUVe9mE/ZAo5ZdPRkWgvumbbGq7i6vD9NAs bYWyL4/yTpiQuJR70Lxr75qjpklYldg6X09SV8viiLrDMI591AwOh3hj4OM6htDEr3ON /+KA== 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=iSQFzh6kLnBCcudLetoO0SevfeUo1ajFojtQyoqXpBQ=; fh=ZM3VoIALjCpG+NKGQ//xV8LjNmVpkgwohdMjQmKLq6Y=; b=LLLG79orY7apG2U71twsXkydsPfI9IvX4xd7ezZvwBrgPk2ZaxqRElApOK7bj7HAqx qvDx9+L9o2IUskKZZQ9lMCFjlUDKupUibulHYh6+jg1g/Vh3DdetO81S/NVtoH7QFuJt pKLoxXV7MSxShxsXeG0NImrH3+5Y8jaiEBYBqgWFQB0OAyZ59rOCRFT/lU4zcYW0EH2X Stc6GP7RiJ+3bzCV+Yu+xp6b74Ue+ojKkAgsiGQckNYzLKQ772py4zdP3QV8Ih1hkQ8k dNl0VRZhl2N0l3ZAGhsl9Y9xYzOFLrbak2Nv7gGxYsXDxsUcCxVJoRpe4gyYwCbx1XyO E1Ng== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ispras.ru header.s=default header.b=qjGGvX+q; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 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 morse.vger.email (morse.vger.email. [23.128.96.31]) by mx.google.com with ESMTPS id g1-20020a635641000000b005b928c7b284si9805074pgm.587.2023.12.05.10.06.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 10:06:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31; Authentication-Results: mx.google.com; dkim=pass header.i=@ispras.ru header.s=default header.b=qjGGvX+q; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 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 morse.vger.email (Postfix) with ESMTP id F0D8380E60A6; Tue, 5 Dec 2023 10:06:00 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345576AbjLESFe (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Tue, 5 Dec 2023 13:05:34 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40504 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230271AbjLESF3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 5 Dec 2023 13:05:29 -0500 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BC6EBB2; Tue, 5 Dec 2023 10:05:34 -0800 (PST) Received: from localhost.localdomain (unknown [46.242.8.170]) by mail.ispras.ru (Postfix) with ESMTPSA id 60EAF40F1DE8; Tue, 5 Dec 2023 18:05:32 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru 60EAF40F1DE8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1701799532; bh=iSQFzh6kLnBCcudLetoO0SevfeUo1ajFojtQyoqXpBQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=qjGGvX+q5/okA2ccONfrW/JIILDtVsCtbi/3lK6KMl7/MKeXrk9Y7g1krpec6W8Q4 MeNVeYPnw637K1E2S+L0ZBw1Sj4vTg4eGXzUDMIoe2stuRvJ82adZVg/+lcNdXhrtu LxQuq8jpwgyfD99rtJTxly4iAOqyk1RvNMLheh3I= From: Fedor Pchelkin <pchelkin@ispras.ru> To: Dominique Martinet <asmadeus@codewreck.org> Cc: Fedor Pchelkin <pchelkin@ispras.ru>, Christian Schoenebeck <linux_oss@crudebyte.com>, Eric Van Hensbergen <ericvh@kernel.org>, Latchesar Ionkov <lucho@ionkov.net>, "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 v3] net: 9p: avoid freeing uninit memory in p9pdu_vreadf Date: Tue, 5 Dec 2023 21:05:22 +0300 Message-ID: <20231205180523.11318-1-pchelkin@ispras.ru> X-Mailer: git-send-email 2.43.0 In-Reply-To: <9f21f00b-0806-4811-8d0a-9b6175eaedeb-pchelkin@ispras.ru> 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 morse.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 (morse.vger.email [0.0.0.0]); Tue, 05 Dec 2023 10:06:01 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784428474878386100 X-GMAIL-MSGID: 1784466179859559359 |
Series |
[v3] net: 9p: avoid freeing uninit memory in p9pdu_vreadf
|
|
Commit Message
Fedor Pchelkin
Dec. 5, 2023, 6:05 p.m. UTC
If some of p9pdu_readf() calls inside case 'T' in p9pdu_vreadf() fails,
the error path is not handled properly. *wnames or members of *wnames
array may be left uninitialized and invalidly freed.
In order not to complicate the code with array index processing, fix the
problem with initializing *wnames to NULL in beginning of case 'T' and
using kcalloc() to allocate and initialize the array. For assurance,
nullify the failing *wnames element (the callee handles that already -
e.g. see 's' case).
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.
v3: Simplify the patch by using kcalloc() instead of array indices
manipulation per Christian Schoenebeck's remark. Update the commit
message accordingly.
net/9p/protocol.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Comments
On Tuesday, December 5, 2023 7:05:22 PM CET Fedor Pchelkin wrote: > If some of p9pdu_readf() calls inside case 'T' in p9pdu_vreadf() fails, > the error path is not handled properly. *wnames or members of *wnames > array may be left uninitialized and invalidly freed. > > In order not to complicate the code with array index processing, fix the > problem with initializing *wnames to NULL in beginning of case 'T' and > using kcalloc() to allocate and initialize the array. For assurance, > nullify the failing *wnames element (the callee handles that already - > e.g. see 's' case). > > 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. > v3: Simplify the patch by using kcalloc() instead of array indices > manipulation per Christian Schoenebeck's remark. Update the commit > message accordingly. > > net/9p/protocol.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > index 4e3a2a1ffcb3..7067fb49d713 100644 > --- a/net/9p/protocol.c > +++ b/net/9p/protocol.c > @@ -394,13 +394,14 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > uint16_t *nwname = va_arg(ap, uint16_t *); > char ***wnames = va_arg(ap, char ***); > > + *wnames = NULL; > + > errcode = p9pdu_readf(pdu, proto_version, > "w", nwname); > if (!errcode) { > *wnames = > - kmalloc_array(*nwname, > - sizeof(char *), > - GFP_NOFS); > + kcalloc(*nwname, sizeof(char *), > + GFP_NOFS); Context of this code is transmitting directory entries, e.g. thousands of array elements. So this would always introduce performance costs. The error cases this patch addresses should happen rather rarely BTW. Another option (instead of clearing the entire array) would be just setting the last entry in the array to NULL, and the loop freeing the elements would stop at the first NULL entry. That way you don't have to worry about carrying `i` along and `i` being correctly intitalized. Would require array size +1 though. In general I agree that this code section calls out to be simplified, but I doubt that clearing the entire array is the best way to go here. > if (!*wnames) > errcode = -ENOMEM; > } > @@ -414,8 +415,10 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > proto_version, > "s", > &(*wnames)[i]); > - if (errcode) > + if (errcode) { > + (*wnames)[i] = NULL; > break; > + } > } > } > > @@ -425,9 +428,9 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > > for (i = 0; i < *nwname; i++) > kfree((*wnames)[i]); > + kfree(*wnames); > + *wnames = NULL; > } > - kfree(*wnames); > - *wnames = NULL; > } > } > break; >
diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 4e3a2a1ffcb3..7067fb49d713 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -394,13 +394,14 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, uint16_t *nwname = va_arg(ap, uint16_t *); char ***wnames = va_arg(ap, char ***); + *wnames = NULL; + errcode = p9pdu_readf(pdu, proto_version, "w", nwname); if (!errcode) { *wnames = - kmalloc_array(*nwname, - sizeof(char *), - GFP_NOFS); + kcalloc(*nwname, sizeof(char *), + GFP_NOFS); if (!*wnames) errcode = -ENOMEM; } @@ -414,8 +415,10 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, proto_version, "s", &(*wnames)[i]); - if (errcode) + if (errcode) { + (*wnames)[i] = NULL; break; + } } } @@ -425,9 +428,9 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, for (i = 0; i < *nwname; i++) kfree((*wnames)[i]); + kfree(*wnames); + *wnames = NULL; } - kfree(*wnames); - *wnames = NULL; } } break;