| Message ID | 20240702093924.12092-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | nfc: pn533: Add poll mod list filling check | expand |
On 02/07/2024 11:39, Aleksandr Mishin wrote: > In case of im_protocols value is 1 and tm_protocols value is 0 this Which im protocol has value 1 in the mask? The pn533_poll_create_mod_list() handles all possible masks, so your case is just not possible to happen. This patch is purely to satisfy (your) static analyzers, so this should be clear in commit msg. You are not fixing any bug but adding sort of defensive code and suppresion of false-positive warning... > combination successfully passes the check > 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). > But then after pn533_poll_create_mod_list() call in pn533_start_poll() > poll mod list will remain empty and dev->poll_mod_count will remain 0 > which lead to division by zero. > > Add poll mod list filling check. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > drivers/nfc/pn533/pn533.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c > index b19c39dcfbd9..e2bc67300a91 100644 > --- a/drivers/nfc/pn533/pn533.c > +++ b/drivers/nfc/pn533/pn533.c > @@ -1723,6 +1723,11 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev, > } > > pn533_poll_create_mod_list(dev, im_protocols, tm_protocols); > + if (!dev->poll_mod_count) { > + nfc_err(dev->dev, > + "Poll mod list is empty\n"); Odd wrapping. > + return -EINVAL; > + } > > /* Do not always start polling from the same modulation */ > get_random_bytes(&rand_mod, sizeof(rand_mod)); Best regards, Krzysztof
On 03.07.2024 8:02, Krzysztof Kozlowski wrote: > On 02/07/2024 11:39, Aleksandr Mishin wrote: >> In case of im_protocols value is 1 and tm_protocols value is 0 this > > Which im protocol has value 1 in the mask? > > The pn533_poll_create_mod_list() handles all possible masks, so your > case is just not possible to happen. Exactly. pn533_poll_create_mod_list() handles all possible specified masks. No im protocol has value 1 in the mask. In case of 'im_protocol' parameter has value of 1, no mod will be added. So dev->poll_mod_count will remain 0. I assume 'im_protocol' parameter is "external" to this driver, it comes from outside and can contain any value, so driver has to be able to protect itself from incorrect values. > > This patch is purely to satisfy (your) static analyzers, so this should > be clear in commit msg. You are not fixing any bug but adding sort of > defensive code and suppresion of false-positive warning... > >> combination successfully passes the check >> 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). >> But then after pn533_poll_create_mod_list() call in pn533_start_poll() >> poll mod list will remain empty and dev->poll_mod_count will remain 0 >> which lead to division by zero. >> >> Add poll mod list filling check. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") >> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >> --- >> drivers/nfc/pn533/pn533.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c >> index b19c39dcfbd9..e2bc67300a91 100644 >> --- a/drivers/nfc/pn533/pn533.c >> +++ b/drivers/nfc/pn533/pn533.c >> @@ -1723,6 +1723,11 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev, >> } >> >> pn533_poll_create_mod_list(dev, im_protocols, tm_protocols); >> + if (!dev->poll_mod_count) { >> + nfc_err(dev->dev, >> + "Poll mod list is empty\n"); > > Odd wrapping. Just like other messages in this function. > >> + return -EINVAL; >> + } >> >> /* Do not always start polling from the same modulation */ >> get_random_bytes(&rand_mod, sizeof(rand_mod)); > > Best regards, > Krzysztof >
On 03/07/2024 09:26, Aleksandr Mishin wrote: > > > On 03.07.2024 8:02, Krzysztof Kozlowski wrote: >> On 02/07/2024 11:39, Aleksandr Mishin wrote: >>> In case of im_protocols value is 1 and tm_protocols value is 0 this >> >> Which im protocol has value 1 in the mask? >> >> The pn533_poll_create_mod_list() handles all possible masks, so your >> case is just not possible to happen. > > Exactly. pn533_poll_create_mod_list() handles all possible specified > masks. No im protocol has value 1 in the mask. In case of 'im_protocol' Which cannot happen. > parameter has value of 1, no mod will be added. So dev->poll_mod_count > will remain 0. Which cannot happen. > I assume 'im_protocol' parameter is "external" to this driver, it comes > from outside and can contain any value, so driver has to be able to > protect itself from incorrect values. Did you read what I wrote? It cannot happen. Best regards, Krzysztof
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c index b19c39dcfbd9..e2bc67300a91 100644 --- a/drivers/nfc/pn533/pn533.c +++ b/drivers/nfc/pn533/pn533.c @@ -1723,6 +1723,11 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev, } pn533_poll_create_mod_list(dev, im_protocols, tm_protocols); + if (!dev->poll_mod_count) { + nfc_err(dev->dev, + "Poll mod list is empty\n"); + return -EINVAL; + } /* Do not always start polling from the same modulation */ get_random_bytes(&rand_mod, sizeof(rand_mod));
In case of im_protocols value is 1 and tm_protocols value is 0 this combination successfully passes the check 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). But then after pn533_poll_create_mod_list() call in pn533_start_poll() poll mod list will remain empty and dev->poll_mod_count will remain 0 which lead to division by zero. Add poll mod list filling check. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- drivers/nfc/pn533/pn533.c | 5 +++++ 1 file changed, 5 insertions(+)