Received: by 2002:a05:7412:8d10:b0:f3:1519:9f41 with SMTP id bj16csp1629200rdb; Thu, 7 Dec 2023 04:54:34 -0800 (PST) X-Google-Smtp-Source: AGHT+IF4qkFUzo++YLlYldmn8vwn6Tdks0GxajcNH4v9BD2z8MLmbFEHR5ZmaLCdhC/g65Y1lclu X-Received: by 2002:a05:6870:f625:b0:1fb:684:220d with SMTP id ek37-20020a056870f62500b001fb0684220dmr2603393oab.48.1701953674559; Thu, 07 Dec 2023 04:54:34 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701953674; cv=none; d=google.com; s=arc-20160816; b=ZsdJNrdCEnwkB/YjeX1/0WnomD1ocmb/vXX3VJNJtLNKsMv8e40o+utcDXK06QTJoY z42ta7mai2q6sUxRfbBvMP2d1ME4mMztpYFi4PdlwVTGeTvgCiPnfdYAIQYLBDw53Wal trsPa0S08qPShw/MPHeQts/802vIIjF05j6RNWwW6qf9WGWIcCJbcRK1U+W+lRsy7vAi bXlFBBSaZodx4d8NsQz3vikf2dVP5Htkzw9yLQlJREgAatBr1M6sFFDFNtrEJau6YYK+ LR4pH9Bg5Sv1CRvu9jrw2ryBTFx7wh8qw0CYrl16JInGs1BPHkOeDn7hwnYZpw/SmCQj 0kVg== 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; bh=xaCvAzlwzR9Hokmynfu7A4/6oLv+b96ic63xDkUsACk=; fh=oZH2VyqwWn+iJTsRgK1LJRCoinvnBAUum7mJf7uazLs=; b=XSktvv2WY/NZekjz+7inV+3ASVNfmMDftrHvW/boyGojFOnDmX3yhFwH9We0HSkiTR YZtXnkyxiMoaCARu/D+TjmADw8t9wDzWjkd1vRsJDLyKfWfiMReHV/Y+YSgHlmukHHik Co4qqRb3EDK2FBe0DN/slSCtXIaEX9kSpqxwUBVVAXWwk3DJZfjPHs1tVDs0BNJLuy60 Ov0VNmnw1Ma8aV6Dp/Ew4q2GgVAaFqez75Pn5N7Yzi25bLVz6pllUiNtelDV1YpqMdpU 9Cy67glGBasvW6WwrS5C4jTpJdMXKyGdbO738sCKs3QxAAtu7arJymZX5Yl/I0v63ygu CW3g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@crudebyte.com header.s=kylie header.b=EEgZ+Cgs; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=crudebyte.com Return-Path: Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id f18-20020a631012000000b005c6666dcce1si1139804pgl.583.2023.12.07.04.54.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Dec 2023 04:54:34 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@crudebyte.com header.s=kylie header.b=EEgZ+Cgs; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=crudebyte.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id B3E178041391; Thu, 7 Dec 2023 04:54:31 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232570AbjLGMyQ (ORCPT + 99 others); Thu, 7 Dec 2023 07:54:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45054 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229472AbjLGMyP (ORCPT ); Thu, 7 Dec 2023 07:54:15 -0500 Received: from kylie.crudebyte.com (kylie.crudebyte.com [5.189.157.229]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 838FCD6D; Thu, 7 Dec 2023 04:54:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=crudebyte.com; s=kylie; h=Content-Type:Content-Transfer-Encoding: MIME-Version:References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From: Content-ID:Content-Description; bh=xaCvAzlwzR9Hokmynfu7A4/6oLv+b96ic63xDkUsACk=; b=EEgZ+CgszC5pK4/GcUfCvYqH04 GLu1k/eCfcwojQHQ5el6pDSztUvnFz19Bl4B2/S8XTbmFiANjFkROQg0dFUlw/NCGnX6MiqL/cvho AB0dPPlwnfe0bwKWgoWkf3FXVQcAZJaTf0OtuMbZxN4DH+978qnPwc8rZ/yZz2U8FZf3f5zp5/nR1 /e/q06i2kmDqEveKXy6S6DcUP7Xgb77s/B5y9AZjjgHPWsR+L2o0LYLxAdIEXJFhle2FemDi0yWy1 0sgqlFw2Eck9usYCTda4MIQPQ6gO4KN5DYfQ4u54ZG2pDBfGxhjrmW/UCNtlAs37a63+MHcfWvsrZ 9h0RGIAgpiD6l0Yf0OyRLhOKreubHCTgeFfmiqM9QyjQgKY2Ck3XY3AXNwUFoxMrf0bgWWps3KOPq EXhaWCwetQmq7zhqlDMndBxh4MjDkjMSc+/+OjAZFmqGcUBqYttE8L7zVKNydG81AjqeEaMwcjOoX DASbpuccxYgxTCA8YPjQKjImXNSk53F8DrMQkrMSo4fhNCeh5ZxzRimcEy+oCTqziTm9Jdv/zX8E4 oapJatLZiXWmQ80xIHNG11D02ziTpiahg61UjfZ6iNF/TXc260fF0beo7BcsT+G6g5l35qcVf1hnb Pn5UT8uk+aLvs2SmkEbRyRDU7kyAEsOIvr8XWcVLw=; From: Christian Schoenebeck To: Dominique Martinet , Fedor Pchelkin Cc: Fedor Pchelkin , Eric Van Hensbergen , Latchesar Ionkov , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , v9fs@lists.linux.dev, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov , lvc-project@linuxtesting.org Subject: Re: [PATCH v4] net: 9p: avoid freeing uninit memory in p9pdu_vreadf Date: Thu, 07 Dec 2023 13:54:02 +0100 Message-ID: <1808202.Umia7laAZq@silver> In-Reply-To: <20231206200913.16135-1-pchelkin@ispras.ru> References: <20231206200913.16135-1-pchelkin@ispras.ru> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 pete.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Thu, 07 Dec 2023 04:54:31 -0800 (PST) On Wednesday, December 6, 2023 9:09:13 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. > > Initialize *wnames to NULL in beginning of case 'T'. Initialize the first > *wnames array element to NULL and nullify the failing *wnames element so > that the error path freeing loop stops on the first NULL element and > doesn't proceed further. > > Found by Linux Verification Center (linuxtesting.org). > > Fixes: ace51c4dd2f9 ("9p: add new protocol support code") > Signed-off-by: Fedor Pchelkin > --- > 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. > v4: Per Christian's suggestion, apply another strategy: mark failing > array element as NULL and move in the freeing loop until it is found. > Update the commit message accordingly. If v4 is more appropriate than the > version at > https://github.com/martinetd/linux/commit/69cc23eb3a0b79538e9b5face200c4cd5cd32ae0 > then please use it, otherwise, I don't think we can provide more > convenient solution here than the one already queued at github. > > net/9p/protocol.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/net/9p/protocol.c b/net/9p/protocol.c > index 4e3a2a1ffcb3..0e6603b1ec90 100644 > --- a/net/9p/protocol.c > +++ b/net/9p/protocol.c > @@ -394,6 +394,8 @@ 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) { > @@ -403,6 +405,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > GFP_NOFS); > if (!*wnames) > errcode = -ENOMEM; > + else > + (*wnames)[0] = NULL; > } > > if (!errcode) { > @@ -414,8 +418,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; > + } I just checked whether this could create a leak, but it looks clean, so LGTM: Reviewed-by: Christian Schoenebeck Dominique, I would tend to use this v4 instead of v2. What do you think? > } > } > > @@ -423,11 +429,14 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, > if (*wnames) { > int i; > > - for (i = 0; i < *nwname; i++) > + for (i = 0; i < *nwname; i++) { > + if (!(*wnames)[i]) > + break; > kfree((*wnames)[i]); > + } > + kfree(*wnames); > + *wnames = NULL; > } > - kfree(*wnames); > - *wnames = NULL; > } > } > break; >