From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ADA1A125D8 for ; Thu, 7 Sep 2023 15:32:19 +0000 (UTC) Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 844001FC1; Thu, 7 Sep 2023 08:31:55 -0700 (PDT) Received: from compute6.internal (compute6.nyi.internal [10.202.2.47]) by mailout.nyi.internal (Postfix) with ESMTP id 41C535C00B0; Thu, 7 Sep 2023 08:42:18 -0400 (EDT) Received: from imap42 ([10.202.2.92]) by compute6.internal (MEProxy); Thu, 07 Sep 2023 08:42:18 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jcline.org; h=cc :cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:sender :subject:subject:to:to; s=fm1; t=1694090538; x=1694176938; bh=UI lscQB4tPzAVE0ptc61SxJC9os7aFiFRvMXeXkTLIM=; b=DwP7eSFhMrJJjUmX3h l1OXxKsGNJp4ATvgVOMuhS7AnL3ONEopuBO2W/0CUYwrkZ94CWlVxN4AXTpbYoY1 9u69NY5vCJTVty3tRZ8iWTjerJb1GTdB+s8XMO4RiPa6v+FuHuinkRRT7lccOlTT T9TqHYWhnx8TrOhK+DxU7PFjaYWi5fDCjcDlz9P0+PDl6eZnE2XlHGfhKUXzpG+M l/L2JTRoKm7QZdxdk4KYV8Jep1JJs7w3d/K40tGx+8AKLGK3qiyW47fIVAZ6noWP RwTyESVsvQQX48bIoU+TNxBVbLSxwQf41Uq2pUeA58bGSvJcJ5M5HikanT0pFHKR gEIg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; t=1694090538; x=1694176938; bh=UIlscQB4tPzAV E0ptc61SxJC9os7aFiFRvMXeXkTLIM=; b=ckZjZP98rzW/Hpq7CqnvHeePkkOuh ESD2C1NhsZ45uV1SHSkTHdwgsa3mebqYsk5N1UOdBBFh3U8uPE4wku83TYeONSE2 Ty2nxHWY4SagAxI3Ta21gOsiPYYmNELkeEAXmbWo5N4csNAP0YqxvzoU10t1LYLa n5tf3BfiAsoRGUlEZ/BcOCM9XNDd7xoc6fBeMgI9XeHSNGBam+D2LMZ+roP8hD6D LUSiaD1WpLEmbFCS/A/sLGmOCfPX65XRWQg1v2Xsn8YsD5/PrDOTT2aduNbWE4Mu N9uqmZ4STGkVNH3izf+MFJRVjeq/q/H5ESq0L27bf32btZSAeAfuof39w== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedviedrudehhedgheegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvfevufgtsehttdertderredtnecuhfhrohhmpedflfgv rhgvmhihucevlhhinhgvfdcuoehjvghrvghmhiesjhgtlhhinhgvrdhorhhgqeenucggtf frrghtthgvrhhnpeeuvedthfdttedttedvgeelleevvdehveejhefgheefuedtleelueek vdeggfeiveenucffohhmrghinhepshihiihkrghllhgvrhdrrghpphhsphhothdrtghomh enucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehjvghr vghmhiesjhgtlhhinhgvrdhorhhg X-ME-Proxy: Feedback-ID: i7a7146c5:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id 6B618BC007C; Thu, 7 Sep 2023 08:42:17 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.9.0-alpha0-711-g440737448e-fm-20230828.001-g44073744 Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Message-Id: In-Reply-To: References: <20230906233347.823171-1-jeremy@jcline.org> Date: Thu, 07 Sep 2023 08:41:56 -0400 From: "Jeremy Cline" To: "Krzysztof Kozlowski" Cc: "David S . Miller" , "Eric Dumazet" , "Jakub Kicinski" , "Paolo Abeni" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, syzbot , "Hillf Danton" Subject: Re: [PATCH] nfc: nci: assert requested protocol is valid Content-Type: text/plain X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Hi, On Thu, Sep 7, 2023, at 2:24 AM, Krzysztof Kozlowski wrote: > On 07/09/2023 01:33, Jeremy Cline wrote: >> The protocol is used in a bit mask to determine if the protocol is >> supported. Assert the provided protocol is less than the maximum >> defined so it doesn't potentially perform a shift-out-of-bounds and >> provide a clearer error for undefined protocols vs unsupported ones. >> >> Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation") >> Reported-and-tested-by: syzbot+0839b78e119aae1fec78@syzkaller.appspotmail.com >> Closes: https://syzkaller.appspot.com/bug?extid=0839b78e119aae1fec78 >> Signed-off-by: Jeremy Cline >> --- >> net/nfc/nci/core.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c >> index fff755dde30d..6c9592d05120 100644 >> --- a/net/nfc/nci/core.c >> +++ b/net/nfc/nci/core.c >> @@ -909,6 +909,11 @@ static int nci_activate_target(struct nfc_dev *nfc_dev, >> return -EINVAL; >> } >> >> + if (protocol >= NFC_PROTO_MAX) { >> + pr_err("the requested nfc protocol is invalid\n"); >> + return -EINVAL; >> + } > > This looks OK, but I wonder if protocol 0 (so BIT(0) in the > supported_protocols) is a valid protocol. I looked at the code and it > was nowhere handled. > I did notice that the protocols started at 1, but I was not particularly confident in adding a check for 0 since I was concerned I might miss a subtle existing case of 0 being used somewhere, or that some time in the future a protocol 0 would be added (which seems weird, but weird things happen I suppose). If it is added in the future and there's a check here marking it invalid explicitly, it will trip up the developer briefly. Since the next check in this function should still reject 0 with an -EINVAL the only downside to not checking is the different error message. I personally lean towards letting the second check catch the 0 case, but as I'm not likely to be the person who has to deal with any of the downsides, I'm happy to do whatever you think is best. Thanks, Jeremy