Re: [PATCH] nfc: nci: assert requested protocol is valid

From: "Jeremy Cline" <jeremy@jcline.org>
To: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
Cc: "David S . Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	syzbot <syzbot+0839b78e119aae1fec78@syzkaller.appspotmail.com>,
	"Hillf Danton" <hdanton@sina.com>
Subject: Re: [PATCH] nfc: nci: assert requested protocol is valid
Date: Thu, 07 Sep 2023 08:41:56 -0400	[thread overview]
Message-ID: <cb4fa1f4-2250-4aad-823f-7cd286f30ccc@app.fastmail.com> (raw)
In-Reply-To: <ccf7072c-cebb-0491-f07e-8c781a2f4664@linaro.org>

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 <jeremy@jcline.org>
>> ---
>>  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