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

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

[Jeremy Cline]

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;
+	}
+
 	if (!(nci_target->supported_protocols & (1 << protocol))) {
 		pr_err("target does not support the requested protocol 0x%x\n",
 		       protocol);
-- 
2.41.0

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

[Krzysztof Kozlowski]

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.

Original patch from Hilf Danton was also handling it (I wonder why Hilf
did not send his patch...)

https://syzkaller.appspot.com/bug?extid=0839b78e119aae1fec78

Best regards,
Krzysztof

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

[Jeremy Cline]

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

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

[Simon Horman]

Cc "John W. Linville" <linville@tuxdriver.com>
   Ilan Elias <ilane@ti.com>
   Dmitry Vyukov <dvyukov@google.com>
   Lin Ma <linma@zju.edu.cn>

On Wed, Sep 06, 2023 at 07:33:47PM -0400, 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>

Hi Jeremy,

As a bug fix, with a Fixes tag, I'm assuming that this targets 'net'.
As opposed to 'net-next'.

There is probably no need to repost for this, but in future please
bear in mind the practice of specifying the target tree in
the subject of Networking patches.

   Subject: [PATCH net] ...

Also, please include addresses indicated by the following in the CC list.

   get_maintainer.pl --min-percent 25 x.patch

The above notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.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;
> +	}
> +
>  	if (!(nci_target->supported_protocols & (1 << protocol))) {
>  		pr_err("target does not support the requested protocol 0x%x\n",
>  		       protocol);
> -- 
> 2.41.0
> 
> 

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

[Jeremy Cline]

On Thu, Sep 07, 2023 at 04:41:12PM +0200, Simon Horman wrote:
> Cc "John W. Linville" <linville@tuxdriver.com>
>    Ilan Elias <ilane@ti.com>
>    Dmitry Vyukov <dvyukov@google.com>
>    Lin Ma <linma@zju.edu.cn>
> 
> On Wed, Sep 06, 2023 at 07:33:47PM -0400, 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>
> 
> Hi Jeremy,
> 
> As a bug fix, with a Fixes tag, I'm assuming that this targets 'net'.
> As opposed to 'net-next'.

Yeah, that's fine, whatever the maintainers think is best works for me.
I'm an infrequent contributor, at best.

> 
> There is probably no need to repost for this, but in future please
> bear in mind the practice of specifying the target tree in
> the subject of Networking patches.
> 
>    Subject: [PATCH net] ...
> 

I'll try to keep that in mind if I send other Networking patches.

> Also, please include addresses indicated by the following in the CC list.
> 
>    get_maintainer.pl --min-percent 25 x.patch
> 

Likewise I'll try to remember this particular version if I send any more
network patches.

> The above notwithstanding, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 

Thanks!

- Jeremy

> > ---
> >  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;
> > +	}
> > +
> >  	if (!(nci_target->supported_protocols & (1 << protocol))) {
> >  		pr_err("target does not support the requested protocol 0x%x\n",
> >  		       protocol);
> > -- 
> > 2.41.0
> > 
> > 

[PATCH v2 net] nfc: nci: assert requested protocol is valid

[Jeremy Cline]

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>
---
Changes from v1:
    - Target the correct tree (net)

 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;
+	}
+
 	if (!(nci_target->supported_protocols & (1 << protocol))) {
 		pr_err("target does not support the requested protocol 0x%x\n",
 		       protocol);
-- 
2.41.0

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

[Simon Horman]

On Mon, Oct 09, 2023 at 04:00:54PM -0400, 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>

As per my review of v1, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon,  9 Oct 2023 16:00:54 -0400 you 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>
> 
> [...]

Here is the summary with links:
  - [v2,net] nfc: nci: assert requested protocol is valid
    https://git.kernel.org/netdev/net/c/354a6e707e29

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html