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(+)

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));
-- 
2.30.2

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
> 

-- 
Kind regards
Aleksandr

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

On Thu, 04. Jul 14:07, Krzysztof Kozlowski wrote:
> 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.

An important thing which unfortunately wasn't mentioned in commit log is
that these protocol values actually come from userspace via Netlink
interface (NFC_CMD_START_POLL operation). So a broken or malicious program
may pass a message containing a "bad" combination of protocol parameter
values so that dev->poll_mod_count is not incremented inside
pn533_poll_create_mod_list(), thus leading to division by zero.

nfc_genl_start_poll()
  nfc_start_poll()
    ->start_poll()
    pn533_start_poll()

Looking at pn533_poll_create_mod_list() source code, seems there may be a
number of such "bad" combinations: e.g. when passed tm_protocols is 0 and
im_protocols is 1.

CAP_NET_ADMIN is currently required to perform device control operations
but it's not a point for the situation to be neglected.

Regarding the patch, maybe it'd be better to include the check inside
pn533_poll_create_mod_list() itself and return an error? That'd be more
convenient if someday this function would be called elsewhere in the code
and dev->poll_mod_count must still be guaranteed to be incremented at
least once.

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.

Normally no im protocol has value 1 in the mask, so this combination is
not expected by driver. But these protocol values actually come from
userspace via Netlink interface (NFC_CMD_START_POLL operation). So a
broken or malicious program may pass a message containing a "bad"
combination of protocol parameter values so that dev->poll_mod_count
is not incremented inside pn533_poll_create_mod_list(), thus leading
to division by zero.
Call trace looks like:
nfc_genl_start_poll()
  nfc_start_poll()
    ->start_poll()
    pn533_start_poll()

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>
---
v2: Update comment message for a more detailed description of the problem

 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");
+		return -EINVAL;
+	}
 
 	/* Do not always start polling from the same modulation */
 	get_random_bytes(&rand_mod, sizeof(rand_mod));
-- 
2.30.2

On 8/27/24 10:48, Aleksandr Mishin wrote:
> 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.
> 
> Normally no im protocol has value 1 in the mask, so this combination is
> not expected by driver. But these protocol values actually come from
> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a
> broken or malicious program may pass a message containing a "bad"
> combination of protocol parameter values so that dev->poll_mod_count
> is not incremented inside pn533_poll_create_mod_list(), thus leading
> to division by zero.
> Call trace looks like:
> nfc_genl_start_poll()
>    nfc_start_poll()
>      ->start_poll()
>      pn533_start_poll()
> 
> 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>

The issue looks real to me and the proposed fix the correct one, but 
waiting a little more for Krzysztof feedback, as he expressed concerns 
on v1.

Thanks,

Paolo

On 29/08/2024 10:26, Paolo Abeni wrote:
> 
> 
> On 8/27/24 10:48, Aleksandr Mishin wrote:
>> 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.
>>
>> Normally no im protocol has value 1 in the mask, so this combination is
>> not expected by driver. But these protocol values actually come from
>> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a
>> broken or malicious program may pass a message containing a "bad"
>> combination of protocol parameter values so that dev->poll_mod_count
>> is not incremented inside pn533_poll_create_mod_list(), thus leading
>> to division by zero.
>> Call trace looks like:
>> nfc_genl_start_poll()
>>    nfc_start_poll()
>>      ->start_poll()
>>      pn533_start_poll()
>>
>> 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>
> 
> The issue looks real to me and the proposed fix the correct one, but 
> waiting a little more for Krzysztof feedback, as he expressed concerns 
> on v1.

There was one month delay between my reply and clarifications from
Fedor, so original patch is neither in my mailbox nor in my brain.


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

However different problem is: shouldn't as well or instead
nfc_genl_start_poll() validate the attributes received by netlink?

We just pass them directly to the drivers and several other drivers
might not expect random stuff there.

Best regards,
Krzysztof

On 8/29/24 11:06, Krzysztof Kozlowski wrote:
> On 29/08/2024 10:26, Paolo Abeni wrote:
>>
>>
>> On 8/27/24 10:48, Aleksandr Mishin wrote:
>>> 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.
>>>
>>> Normally no im protocol has value 1 in the mask, so this combination is
>>> not expected by driver. But these protocol values actually come from
>>> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a
>>> broken or malicious program may pass a message containing a "bad"
>>> combination of protocol parameter values so that dev->poll_mod_count
>>> is not incremented inside pn533_poll_create_mod_list(), thus leading
>>> to division by zero.
>>> Call trace looks like:
>>> nfc_genl_start_poll()
>>>     nfc_start_poll()
>>>       ->start_poll()
>>>       pn533_start_poll()
>>>
>>> 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>
>>
>> The issue looks real to me and the proposed fix the correct one, but
>> waiting a little more for Krzysztof feedback, as he expressed concerns
>> on v1.
> 
> There was one month delay between my reply and clarifications from
> Fedor, so original patch is neither in my mailbox nor in my brain.
> 
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> However different problem is: shouldn't as well or instead
> nfc_genl_start_poll() validate the attributes received by netlink?
> 
> We just pass them directly to the drivers and several other drivers
> might not expect random stuff there.

FTR, I had a similar thought and skimmed over other nfc drivers. I did 
not see similar issues there.

Additionally I fear that existing user-space could feed to the kernel 
such random stuff and work happily because the kernel is currently 
ignoring it - on other drivers. Such cases will suddenly stop working.

I think we could/should merge the patch as-is, please LMK your thought.

Thanks,

Paolo

On 8/29/24 11:06, Krzysztof Kozlowski wrote:
> On 29/08/2024 10:26, Paolo Abeni wrote:
>>
>>
>> On 8/27/24 10:48, Aleksandr Mishin wrote:
>>> 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.
>>>
>>> Normally no im protocol has value 1 in the mask, so this combination is
>>> not expected by driver. But these protocol values actually come from
>>> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a
>>> broken or malicious program may pass a message containing a "bad"
>>> combination of protocol parameter values so that dev->poll_mod_count
>>> is not incremented inside pn533_poll_create_mod_list(), thus leading
>>> to division by zero.
>>> Call trace looks like:
>>> nfc_genl_start_poll()
>>>     nfc_start_poll()
>>>       ->start_poll()
>>>       pn533_start_poll()
>>>
>>> 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>
>>
>> The issue looks real to me and the proposed fix the correct one, but
>> waiting a little more for Krzysztof feedback, as he expressed concerns
>> on v1.
> 
> There was one month delay between my reply and clarifications from
> Fedor, so original patch is neither in my mailbox nor in my brain.
> 
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> However different problem is: shouldn't as well or instead
> nfc_genl_start_poll() validate the attributes received by netlink?
> 
> We just pass them directly to the drivers and several other drivers
> might not expect random stuff there.

FTR, I had a similar thought and skimmed over other nfc drivers. I did 
not see similar issues there.

Additionally I fear that existing user-space could feed to the kernel 
such random stuff and work happily because the kernel is currently 
ignoring it - on other drivers. Such cases will suddenly stop working.

I think we could/should merge the patch as-is, please LMK your thought.

Thanks,

Paolo

On 29/08/2024 11:34, Paolo Abeni wrote:
> On 8/29/24 11:06, Krzysztof Kozlowski wrote:
>> On 29/08/2024 10:26, Paolo Abeni wrote:
>>>
>>>
>>> On 8/27/24 10:48, Aleksandr Mishin wrote:
>>>> 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.
>>>>
>>>> Normally no im protocol has value 1 in the mask, so this combination is
>>>> not expected by driver. But these protocol values actually come from
>>>> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a
>>>> broken or malicious program may pass a message containing a "bad"
>>>> combination of protocol parameter values so that dev->poll_mod_count
>>>> is not incremented inside pn533_poll_create_mod_list(), thus leading
>>>> to division by zero.
>>>> Call trace looks like:
>>>> nfc_genl_start_poll()
>>>>     nfc_start_poll()
>>>>       ->start_poll()
>>>>       pn533_start_poll()
>>>>
>>>> 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>
>>>
>>> The issue looks real to me and the proposed fix the correct one, but
>>> waiting a little more for Krzysztof feedback, as he expressed concerns
>>> on v1.
>>
>> There was one month delay between my reply and clarifications from
>> Fedor, so original patch is neither in my mailbox nor in my brain.
>>
>>
>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> However different problem is: shouldn't as well or instead
>> nfc_genl_start_poll() validate the attributes received by netlink?
>>
>> We just pass them directly to the drivers and several other drivers
>> might not expect random stuff there.
> 
> FTR, I had a similar thought and skimmed over other nfc drivers. I did 
> not see similar issues there.
> 
> Additionally I fear that existing user-space could feed to the kernel 
> such random stuff and work happily because the kernel is currently 
> ignoring it - on other drivers. Such cases will suddenly stop working.
> 
> I think we could/should merge the patch as-is, please LMK your thought.

Yeah, the patch I already acked, can go in. I am just thinking whether
we need a follow-up for core NFC code.

Best regards,
Krzysztof

On 8/29/24 10:26, Paolo Abeni wrote:
> The issue looks real to me and the proposed fix the correct one, but
> waiting a little more for Krzysztof feedback, as he expressed concerns
> on v1.

I almost forgot: for next submissions, please include the target tree in 
the subj prefix ('net' in this case) and avoid reply to the same thread 
with the new version: that usually confuses the patchbot.

Thanks,

Paolo