[<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
Powered by blists - more mailing lists