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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <cb4fa1f4-2250-4aad-823f-7cd286f30ccc@app.fastmail.com>
Date: Thu, 07 Sep 2023 08:41:56 -0400
From: "Jeremy Cline" <jeremy@...ine.org>
To: "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>
Cc: "David S . Miller" <davem@...emloft.net>,
 "Eric Dumazet" <edumazet@...gle.com>, "Jakub Kicinski" <kuba@...nel.org>,
 "Paolo Abeni" <pabeni@...hat.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 syzbot <syzbot+0839b78e119aae1fec78@...kaller.appspotmail.com>,
 "Hillf Danton" <hdanton@...a.com>
Subject: Re: [PATCH] nfc: nci: assert requested protocol is valid

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@...kaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=0839b78e119aae1fec78
>> Signed-off-by: Jeremy Cline <jeremy@...ine.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