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