[PATCH] net: nfc: nci: fix for UBSAN: shift-out-of-bounds in nci_activate_target

[PATCH] net: nfc: nci: fix for UBSAN: shift-out-of-bounds in nci_activate_target

[Anup Sharma]

syzbot found  UBSAN: shift-out-of-bounds in nci_activate_target [1],
when nci_target->supported_protocols is bigger than UNIT_MAX,
where supported_protocols is unsigned 32-bit interger type.

32 is the maximum allowed for supported_protocols. Added a check
for it. 

[1] UBSAN: shift-out-of-bounds in net/nfc/nci/core.c:912:45
shift exponent 4294967071 is too large for 32-bit type 'int'
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x136/0x150 lib/dump_stack.c:106
 ubsan_epilogue lib/ubsan.c:217 [inline]
 __ubsan_handle_shift_out_of_bounds+0x221/0x5a0 lib/ubsan.c:387
 nci_activate_target.cold+0x1a/0x1f net/nfc/nci/core.c:912
 nfc_activate_target+0x1f8/0x4c0 net/nfc/core.c:420
 nfc_genl_activate_target+0x1f3/0x290 net/nfc/netlink.c:900
 genl_family_rcv_msg_doit.isra.0+0x1e6/0x2d0 net/netlink/genetlink.c:968
 genl_family_rcv_msg net/netlink/genetlink.c:1048 [inline]

Reported-by: syzbot+0839b78e119aae1fec78@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=19cf2724120ef8c51c8d2566df0cc34617188433

Signed-off-by: anupsharma <anupnewsmail@gmail.com>
---
 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..e9d968bd1cd9 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -908,6 +908,11 @@ static int nci_activate_target(struct nfc_dev *nfc_dev,
 		pr_err("unable to find the selected target\n");
 		return -EINVAL;
 	}
+	
+	if (nci_target->supported_protocols >= 32) {
+		pr_err("Too many supported protocol by the device\n");
+		return -EINVAL;
+	}
 
 	if (!(nci_target->supported_protocols & (1 << protocol))) {
 		pr_err("target does not support the requested protocol 0x%x\n",
-- 
2.34.1

Re: [PATCH] net: nfc: nci: fix for UBSAN: shift-out-of-bounds in nci_activate_target

[Krzysztof Kozlowski]

On 19/04/2023 03:16, Anup Sharma wrote:
> syzbot found  UBSAN: shift-out-of-bounds in nci_activate_target [1],
> when nci_target->supported_protocols is bigger than UNIT_MAX,

UINT_MAX?

> where supported_protocols is unsigned 32-bit interger type.

integer?

> 
> 32 is the maximum allowed for supported_protocols. Added a check
> for it. 
> 
> [1] UBSAN: shift-out-of-bounds in net/nfc/nci/core.c:912:45
> shift exponent 4294967071 is too large for 32-bit type 'int'
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0x136/0x150 lib/dump_stack.c:106
>  ubsan_epilogue lib/ubsan.c:217 [inline]
>  __ubsan_handle_shift_out_of_bounds+0x221/0x5a0 lib/ubsan.c:387
>  nci_activate_target.cold+0x1a/0x1f net/nfc/nci/core.c:912
>  nfc_activate_target+0x1f8/0x4c0 net/nfc/core.c:420
>  nfc_genl_activate_target+0x1f3/0x290 net/nfc/netlink.c:900
>  genl_family_rcv_msg_doit.isra.0+0x1e6/0x2d0 net/netlink/genetlink.c:968
>  genl_family_rcv_msg net/netlink/genetlink.c:1048 [inline]
> 
> Reported-by: syzbot+0839b78e119aae1fec78@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=19cf2724120ef8c51c8d2566df0cc34617188433
> 
> Signed-off-by: anupsharma <anupnewsmail@gmail.com>
> ---
>  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..e9d968bd1cd9 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -908,6 +908,11 @@ static int nci_activate_target(struct nfc_dev *nfc_dev,
>  		pr_err("unable to find the selected target\n");
>  		return -EINVAL;
>  	}
> +	
> +	if (nci_target->supported_protocols >= 32) {

I don't think it makes any sense. How do you protect from UBSAN reported
shift? Why supported_protocols cannot be 33? You are not shifting the
supported_protocols...

> +		pr_err("Too many supported protocol by the device\n");
> +		return -EINVAL;

I am pretty sure that you broke now NFC. Test the patches first and
share your test scenario.

> +	}
>  
>  	if (!(nci_target->supported_protocols & (1 << protocol))) {



Best regards,
Krzysztof

Re: [PATCH] net: nfc: nci: fix for UBSAN: shift-out-of-bounds in nci_activate_target

[Krzysztof Kozlowski]

On 19/04/2023 03:16, Anup Sharma wrote:
> syzbot found  UBSAN: shift-out-of-bounds in nci_activate_target [1],
> when nci_target->supported_protocols is bigger than UNIT_MAX,
> where supported_protocols is unsigned 32-bit interger type.
> 
> 32 is the maximum allowed for supported_protocols. Added a check
> for it. 
> 
> [1] UBSAN: shift-out-of-bounds in net/nfc/nci/core.c:912:45
> shift exponent 4294967071 is too large for 32-bit type 'int'
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0x136/0x150 lib/dump_stack.c:106
>  ubsan_epilogue lib/ubsan.c:217 [inline]
>  __ubsan_handle_shift_out_of_bounds+0x221/0x5a0 lib/ubsan.c:387
>  nci_activate_target.cold+0x1a/0x1f net/nfc/nci/core.c:912
>  nfc_activate_target+0x1f8/0x4c0 net/nfc/core.c:420
>  nfc_genl_activate_target+0x1f3/0x290 net/nfc/netlink.c:900
>  genl_family_rcv_msg_doit.isra.0+0x1e6/0x2d0 net/netlink/genetlink.c:968
>  genl_family_rcv_msg net/netlink/genetlink.c:1048 [inline]
> 
> Reported-by: syzbot+0839b78e119aae1fec78@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=19cf2724120ef8c51c8d2566df0cc34617188433
> 
> Signed-off-by: anupsharma <anupnewsmail@gmail.com>

Also, no blank lines between tags.

Best regards,
Krzysztof

Re: [PATCH] net: nfc: nci: fix for UBSAN: shift-out-of-bounds in nci_activate_target

[Krzysztof Kozlowski]

On 19/04/2023 10:26, Krzysztof Kozlowski wrote:
> On 19/04/2023 03:16, Anup Sharma wrote:
>> syzbot found  UBSAN: shift-out-of-bounds in nci_activate_target [1],
>> when nci_target->supported_protocols is bigger than UNIT_MAX,
> 
> UINT_MAX?
> 
>> where supported_protocols is unsigned 32-bit interger type.
> 
> integer?
> 
>>
>> 32 is the maximum allowed for supported_protocols. Added a check
>> for it. 
>>
>> [1] UBSAN: shift-out-of-bounds in net/nfc/nci/core.c:912:45
>> shift exponent 4294967071 is too large for 32-bit type 'int'
>> Call Trace:
>>  <TASK>
>>  __dump_stack lib/dump_stack.c:88 [inline]
>>  dump_stack_lvl+0x136/0x150 lib/dump_stack.c:106
>>  ubsan_epilogue lib/ubsan.c:217 [inline]
>>  __ubsan_handle_shift_out_of_bounds+0x221/0x5a0 lib/ubsan.c:387
>>  nci_activate_target.cold+0x1a/0x1f net/nfc/nci/core.c:912
>>  nfc_activate_target+0x1f8/0x4c0 net/nfc/core.c:420
>>  nfc_genl_activate_target+0x1f3/0x290 net/nfc/netlink.c:900
>>  genl_family_rcv_msg_doit.isra.0+0x1e6/0x2d0 net/netlink/genetlink.c:968
>>  genl_family_rcv_msg net/netlink/genetlink.c:1048 [inline]
>>
>> Reported-by: syzbot+0839b78e119aae1fec78@syzkaller.appspotmail.com
>> Link: https://syzkaller.appspot.com/bug?id=19cf2724120ef8c51c8d2566df0cc34617188433
>>
>> Signed-off-by: anupsharma <anupnewsmail@gmail.com>
>> ---
>>  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..e9d968bd1cd9 100644
>> --- a/net/nfc/nci/core.c
>> +++ b/net/nfc/nci/core.c
>> @@ -908,6 +908,11 @@ static int nci_activate_target(struct nfc_dev *nfc_dev,
>>  		pr_err("unable to find the selected target\n");
>>  		return -EINVAL;
>>  	}
>> +	
>> +	if (nci_target->supported_protocols >= 32) {
> 
> I don't think it makes any sense. How do you protect from UBSAN reported
> shift? Why supported_protocols cannot be 33? You are not shifting the
> supported_protocols...
> 
>> +		pr_err("Too many supported protocol by the device\n");
>> +		return -EINVAL;
> 
> I am pretty sure that you broke now NFC. Test the patches first and
> share your test scenario.

BTW, ISO15693 is here protocol 128, so definitely more than 32.

Best regards,
Krzysztof

Re: [PATCH] net: nfc: nci: fix for UBSAN: shift-out-of-bounds in nci_activate_target

[Krzysztof Kozlowski]

On 19/04/2023 21:21, Anup Sharma wrote:

> 
>>>> +		pr_err("Too many supported protocol by the device\n");
>>>> +		return -EINVAL;
>>>
>>> I am pretty sure that you broke now NFC. Test the patches first and
>>> share your test scenario.
> 
> Since a reproducer for this bug is not available, I am unable to test it locally
> or through syzbot before submitting the patch. 

Reproducer is only to test the actual issue, not test the code. Code can
be tested with real device and maybe with virtual NCI driver.

> Are there any other tests that
> I can perform before submitting the patch, apart from simply compiling the kernel?

Compiling a kernel is not tested. Maybe you can test this part
successfully with virtual NCI driver, maybe not, I don't know.

> 
>>
>> BTW, ISO15693 is here protocol 128, so definitely more than 32.
> 
> Thank you for your feedback. I would like to address the UBSAN bug and I have
> thought of a potential solution which involves adding a check statement for the
> minimum and maximum values of the protocol before net/nfc/nci/core.c +912:
> 
> if (!(nci_target->supported_protocols & (1 << protocol)))
> 
> Could you please assist me in determining the correct approach?

I would first propose to check whether the UBSAN report is an actual
real issue to fix.


Best regards,
Krzysztof