| Message ID | 20230906233347.823171-1-jeremy@jcline.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | nfc: nci: assert requested protocol is valid | expand |
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
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
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 > >
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 > > > >
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);
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(+)