* [PATCH] nfc: nci: assert requested protocol is valid
@ 2023-09-06 23:33 Jeremy Cline
2023-09-07 6:24 ` Krzysztof Kozlowski
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Jeremy Cline @ 2023-09-06 23:33 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, Jeremy Cline, syzbot+0839b78e119aae1fec78
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] nfc: nci: assert requested protocol is valid 2023-09-06 23:33 [PATCH] nfc: nci: assert requested protocol is valid Jeremy Cline @ 2023-09-07 6:24 ` Krzysztof Kozlowski 2023-09-07 12:41 ` Jeremy Cline 2023-09-07 14:41 ` Simon Horman 2023-10-09 20:00 ` [PATCH v2 net] " Jeremy Cline 2 siblings, 1 reply; 8+ messages in thread From: Krzysztof Kozlowski @ 2023-09-07 6:24 UTC (permalink / raw) To: Jeremy Cline Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, syzbot+0839b78e119aae1fec78, Hillf Danton 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfc: nci: assert requested protocol is valid 2023-09-07 6:24 ` Krzysztof Kozlowski @ 2023-09-07 12:41 ` Jeremy Cline 0 siblings, 0 replies; 8+ messages in thread From: Jeremy Cline @ 2023-09-07 12:41 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, syzbot, Hillf Danton 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfc: nci: assert requested protocol is valid 2023-09-06 23:33 [PATCH] nfc: nci: assert requested protocol is valid Jeremy Cline 2023-09-07 6:24 ` Krzysztof Kozlowski @ 2023-09-07 14:41 ` Simon Horman 2023-09-08 1:01 ` Jeremy Cline 2023-10-09 20:00 ` [PATCH v2 net] " Jeremy Cline 2 siblings, 1 reply; 8+ messages in thread From: Simon Horman @ 2023-09-07 14:41 UTC (permalink / raw) To: Jeremy Cline Cc: Krzysztof Kozlowski, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, syzbot+0839b78e119aae1fec78, John W. Linville, Ilan Elias, Dmitry Vyukov, Lin Ma 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfc: nci: assert requested protocol is valid 2023-09-07 14:41 ` Simon Horman @ 2023-09-08 1:01 ` Jeremy Cline 0 siblings, 0 replies; 8+ messages in thread From: Jeremy Cline @ 2023-09-08 1:01 UTC (permalink / raw) To: Simon Horman Cc: Krzysztof Kozlowski, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, syzbot+0839b78e119aae1fec78, John W. Linville, Ilan Elias, Dmitry Vyukov, Lin Ma 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 > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 net] nfc: nci: assert requested protocol is valid 2023-09-06 23:33 [PATCH] nfc: nci: assert requested protocol is valid Jeremy Cline 2023-09-07 6:24 ` Krzysztof Kozlowski 2023-09-07 14:41 ` Simon Horman @ 2023-10-09 20:00 ` Jeremy Cline 2023-10-11 15:37 ` Simon Horman 2023-10-12 7:40 ` patchwork-bot+netdevbpf 2 siblings, 2 replies; 8+ messages in thread From: Jeremy Cline @ 2023-10-09 20:00 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Dmitry Vyukov, Lin Ma, Ilan Elias, John W . Linville, netdev, linux-kernel, Simon Horman, Jeremy Cline, syzbot+0839b78e119aae1fec78 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 net] nfc: nci: assert requested protocol is valid 2023-10-09 20:00 ` [PATCH v2 net] " Jeremy Cline @ 2023-10-11 15:37 ` Simon Horman 2023-10-12 7:40 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 8+ messages in thread From: Simon Horman @ 2023-10-11 15:37 UTC (permalink / raw) To: Jeremy Cline Cc: Krzysztof Kozlowski, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Dmitry Vyukov, Lin Ma, Ilan Elias, John W . Linville, netdev, linux-kernel, syzbot+0839b78e119aae1fec78 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 net] nfc: nci: assert requested protocol is valid 2023-10-09 20:00 ` [PATCH v2 net] " Jeremy Cline 2023-10-11 15:37 ` Simon Horman @ 2023-10-12 7:40 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 8+ messages in thread From: patchwork-bot+netdevbpf @ 2023-10-12 7:40 UTC (permalink / raw) To: Jeremy Cline Cc: krzysztof.kozlowski, davem, edumazet, kuba, pabeni, dvyukov, linma, ilane, linville, netdev, linux-kernel, horms, syzbot+0839b78e119aae1fec78 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-10-12 7:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-06 23:33 [PATCH] nfc: nci: assert requested protocol is valid Jeremy Cline 2023-09-07 6:24 ` Krzysztof Kozlowski 2023-09-07 12:41 ` Jeremy Cline 2023-09-07 14:41 ` Simon Horman 2023-09-08 1:01 ` Jeremy Cline 2023-10-09 20:00 ` [PATCH v2 net] " Jeremy Cline 2023-10-11 15:37 ` Simon Horman 2023-10-12 7:40 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.