Received: by 2002:a05:6358:700f:b0:131:369:b2a3 with SMTP id 15csp2519962rwo; Thu, 3 Aug 2023 10:30:43 -0700 (PDT) X-Google-Smtp-Source: APBJJlGRj8tXium7TOy9qvQjwuRuA3mGH94/VGMQKmFeSsztSviUri5ylULyhOdo1UBJORNXoB61 X-Received: by 2002:a19:3803:0:b0:4fb:9469:d65f with SMTP id f3-20020a193803000000b004fb9469d65fmr7201721lfa.37.1691083842818; Thu, 03 Aug 2023 10:30:42 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1691083842; cv=none; d=google.com; s=arc-20160816; b=n2km1KkaerZbsD0JA2QRAljHQfQCziqt9+f8PwIQUOrcJNsorBvdPttNgsog8oRDIM 0Ci0LwEaKkutYrsx4DWv+qtiUSxE4gCugGWSQBg3s/6CbOxSd5H2KZh0LWE0ZjpUOzQK 2qDBZSUXEqdbaRYeqcSyTGlf3S9O28JRuh8QgQdVNHV6pHUsOUhb66K/JEoFeD+SqFWG H3H9ogHfKE5KrEhWqzP45oqPaO8+gWU88uHIfHg8YnBFXY2J6S3phmhgmY8/15Cjip4s LJtT1Ek86f+a0sOvVFPjIUuXAqbfA2oKtqbNviHNdRdjZIJwjfAwGk9VDCe8jdhUoQYX CxlA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:user-agent :content-transfer-encoding:references:in-reply-to:date:cc:to:from :subject:message-id:dkim-signature; bh=ttGwO2VZQ4wu2e0TsIHd2BjnK1hGwLTfixSD0jiL4Mw=; fh=60E6imqibydzL9gr0QBjrEv6q7d5uD2iBocP2yB7KpI=; b=Nz7Tk8iAp1LjR5oYZefqkcgotFTly9YIAYKCjP/xDz2uOv4+HbyXbPd3DrS1TD+8Fe skPz8k3f5x4Wn4zXVccbs5XIkq493Q+YhY5i9GRvlK531hpn0UBPNUnib/xx1BM2CW3c kiBfwOS4gIj2aKrs/mtOiOumOlvpVE2ldSgyCsza1lcOKVlKkQzIRToZpBL+gMnxyYpJ sFEgu3ko1PoKNd8TVrFjJPgn/ZsGuP/ZWRTmluyIXOmv/iHxLgFFbdKF70D866I9mU8U 1g1YqSl/sh2NRl0mv0+mH+qwK4W75gjVbR8CBScGitbcB88dmA6rAooQeVC0k55kERYJ YfoA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=lPWwi0K5; 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; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id x22-20020aa7d6d6000000b00522282c8c31si109014edr.649.2023.08.03.10.30.18; Thu, 03 Aug 2023 10:30:42 -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; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=lPWwi0K5; 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; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234536AbjHCQ6K (ORCPT + 99 others); Thu, 3 Aug 2023 12:58:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51174 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234689AbjHCQ5c (ORCPT ); Thu, 3 Aug 2023 12:57:32 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BE4D346B0; Thu, 3 Aug 2023 09:56:58 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 2262961E5A; Thu, 3 Aug 2023 16:56:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 034BDC433C7; Thu, 3 Aug 2023 16:56:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1691081816; bh=fx8WknJyDBVEgJv4NSl2bGwv0NUpl9D+DCz9SJGxGfY=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=lPWwi0K5zRv8DmFUo52FMpfa4fBoMQk1vZz2JRMuTbWEpre2JfWUABPdS2Yj7mv38 wXSbG+Flk5O53O7uW9/qco48Ib4MYe1J+G0TKHWaOwU0sKgIdrhLR3fSjPNJTrfQVO N1D9CDZeiw3V7G1HLp/qYIWL77vs8BVUUKn4SRkBZYTl6KNYswW+Ny8MAM/KdaMPJy ukFrx7sb56zXI73nOwLJ5AR1wtt4+yNF1xDgwQlhAcmFcxHtZgokPAr4d/pR7QMwS8 Mse6CBIHRbo83rzgPO1jcNL9gk5vcfWm6XOvByZjZvKstplt9QBbrXDXa/HPQGfz42 /rnwSchaQM/NQ== Message-ID: <222fa7b5df94a6e0898612ec62d2e6d2f4163cb8.camel@kernel.org> Subject: Re: [PATCH] fs: lockd: avoid possible wrong NULL parameter From: Jeff Layton To: Nick Desaulniers , Su Hui Cc: Dan Carpenter , chuck.lever@oracle.com, neilb@suse.de, kolga@netapp.com, Dai.Ngo@oracle.com, tom@talpey.com, trond.myklebust@hammerspace.com, anna@kernel.org, nathan@kernel.org, trix@redhat.com, bfields@fieldses.org, linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org, llvm@lists.linux.dev, kernel-janitors@vger.kernel.org Date: Thu, 03 Aug 2023 12:56:53 -0400 In-Reply-To: References: <5066885c-3ac9-adbd-6852-eba89657470c@nfschina.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) MIME-Version: 1.0 X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, 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: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2023-08-03 at 09:38 -0700, Nick Desaulniers wrote: > On Wed, Aug 2, 2023 at 9:11=E2=80=AFPM Su Hui wrote: > >=20 > > On 2023/8/3 05:40, Nick Desaulniers wrote: > >=20 > > On Wed, Aug 2, 2023 at 3:25=E2=80=AFAM Dan Carpenter wrote: > >=20 > >=20 > > I noticed that the function in question already has a guard: > > 322 if (hostname && memchr(hostname, '/', hostname_len) !=3D NULL) { > >=20 > > Which implies that hostname COULD be NULL. > >=20 > > Should this perhaps simply be rewritten as: > >=20 > > if (!hostname) > > return NULL; > > if (memchr(...) !=3D NULL) > > ... > >=20 > > Rather than bury yet another guard for the same check further down in > > the function? Check once and bail early. > >=20 > > Hi, Nick Desaulnier, > >=20 > > This may disrupt the logic of this function. So maybe the past one is b= etter. > >=20 > > 322 if (!hostname) > > 323 return NULL; > > ^^^^^^^^^^^^ > > If we return in this place. > >=20 > > 324 if (memchr(hostname, '/', hostname_len) !=3D NULL) { > > 325 if (printk_ratelimit()) { > > 326 printk(KERN_WARNING "Invalid hostname \"%.*= s\" " > > 327 "in NFS lock request\n"= , > > 328 (int)hostname_len, hostname); > > 329 } > > 330 return NULL; > > 331 } > > 332 > > 333 retry: > > 334 spin_lock(&nsm_lock); > > 335 > > 336 if (nsm_use_hostnames && hostname !=3D NULL) > > 337 cached =3D nsm_lookup_hostname(&ln->nsm_handles, > > 338 hostname, hostname_len); > > 339 else > > 340 cached =3D nsm_lookup_addr(&ln->nsm_handles, sap); > > ^^^^^^^^^^^^^^^ > > This case will be broken when hostname is NULL. >=20 > Ah, you're right; I agree. >=20 > What are your thoughts then about moving the "is hostname NULL" check > to nsm_create_handle() then? >=20 > Perhaps nsm_create_handle() should check that immediately and return > NULL, or simply skip the memcpy if hostname is NULL? >=20 > It seems perhaps best to localize this to the callee rather than the > caller. While there is only one caller today, should another arise > someday, they may make the same mistake. >=20 > I don't feel strongly either way, and am happy to sign off on either > approach; just triple checking we've considered a few different cases. >=20 I think that sounds like the best fix here. Just have nsm_create_handle return NULL immediately if hostname is NULL. > >=20 > > 341 > > 342 if (cached !=3D NULL) { > > 343 refcount_inc(&cached->sm_count); > > 344 spin_unlock(&nsm_lock); > > 345 kfree(new); > > 346 dprintk("lockd: found nsm_handle for %s (%s), " > > 347 "cnt %d\n", cached->sm_name, > > 348 cached->sm_addrbuf, > > 349 refcount_read(&cached->sm_count)); > > 350 return cached; > > 351 } > >=20 >=20 >=20 --=20 Jeff Layton