Re: 5.10.58 UBSAN from brcmf_sdio_dpc+0xa50/0x128c [brcmfmac]

Re: 5.10.58 UBSAN from brcmf_sdio_dpc+0xa50/0x128c [brcmfmac]

[Arend van Spriel]

+netdev, +Dave

On 8/17/2021 7:42 AM, Arend van Spriel wrote:
> Using different email to avoid disclaimers...
> 
> 
> On August 17, 2021 2:39:56 AM Ryutaroh Matsumoto 
> <ryutaroh@ict.e.titech.ac.jp> wrote:
> 
>> Hi Arend, thank you for paying attention to this.
>>
>>> Line 2016 in skbuff.h is inline function __skb_queue_before() and as
>>> far as I can tell brcmfmac is not using that direct or indirect. Maybe
>>> I am reading the line info incorrectly?
>>
>> I am unsure of it. On the other hand, I have also seen somewhat similar
>> UBSAN from a header file "include/net/flow.h" as reported at
>> https://lore.kernel.org/netdev/20210813.081908.1574714532738245424.ryutaroh@ict.e.titech.ac.jp/ 
>>
>>
>> All UBSANs that I have seen come from *.h compiled with clang...
>>
>>> Would you be able to provide information as to what line
>>> brcmf_sdio_dpc+0xa50 refers to.
>>
>> I'd like to do, but I do not know how to let kernel UBSAN include a 
>> line number,
>> though I know it with user-space applications...
> 
> If you enable CONFIG_DEBUG_INFO in your kernel .config and recompile 
> brcmfmac you can load the module in gdb:
> 
> gdb> add-symbol-file brcmfmac.ko [address]
> gdb> l *brcmf_sdio_dpc+0xa50
> 
> The [address] is not very important so just fill in a nice value. The 
> 'l' command should provide the line number.

Hi Ryutaroh,

Meanwhile I did some digging in the brcmfmac driver and I think I found 
the location in brcmf_sdio_sendfromq() where we do a __skb_queue_tail(). 
So I looked at that and it does following:

static inline void __skb_queue_tail(struct sk_buff_head *list,
				   struct sk_buff *newsk)
{
	__skb_queue_before(list, (struct sk_buff *)list, newsk);
}

Your report seems to be coming from the cast that is done here, which is 
fine as long as sk_buff and sk_buff_head have the same members 'next' 
and 'prev' at the start, which is true today and hopefully forever ;-) I 
am inclined to say this is a false report.

Can you please confirm the stack trace indeed points to 
brcmf_sdio_sendfromq() in your report.

Regards,
Arend

Re: 5.10.58 UBSAN from brcmf_sdio_dpc+0xa50/0x128c [brcmfmac]

[Ryutaroh Matsumoto]

Hi Arend, sorry for my late response, partly because compilation takes 10 hours
on RPi4B...
I did the same thing with version 5.10.59 and CLang 11 and got the same
UBSAN. 

>> If you enable CONFIG_DEBUG_INFO in your kernel .config and recompile
>> brcmfmac you can load the module in gdb:
>> gdb> add-symbol-file brcmfmac.ko [address]
>> gdb> l *brcmf_sdio_dpc+0xa50
>> The [address] is not very important so just fill in a nice value. The
>> 'l' command should provide the line number.
> 
> Hi Ryutaroh,
> 
> Meanwhile I did some digging in the brcmfmac driver and I think I
> found the location in brcmf_sdio_sendfromq() where we do a
> __skb_queue_tail(). So I looked at that and it does following:
> 
> static inline void __skb_queue_tail(struct sk_buff_head *list,
> 				   struct sk_buff *newsk)
> {
> 	__skb_queue_before(list, (struct sk_buff *)list, newsk);
> }
> 
> Your report seems to be coming from the cast that is done here, which
> is fine as long as sk_buff and sk_buff_head have the same members
> 'next' and 'prev' at the start, which is true today and hopefully
> forever ;-) I am inclined to say this is a false report.
> 
> Can you please confirm the stack trace indeed points to
> brcmf_sdio_sendfromq() in your report.

Summary: I confirm that the stack trace indeed points to
brcmf_sdio_sendfromq(). The detail follows (you don't have to read it if
you believe in me :-)
If kernel version 5.10.x or 5.13.x is compiled with gcc 10, I have never seen
UBSAN. I wonder if CLang 11/12 tends to generate falsely positive UBSAN.

(gdb) add-symbol-file  drivers/net/wireless/broadcom/brcm80211/brcmfmac/brcmfmac.ko 0x0
add symbol table from file "drivers/net/wireless/broadcom/brcm80211/brcmfmac/brcmfmac.ko" at
	.text_addr = 0x0
(y or n) y

(gdb)  l *brcmf_sdio_dpc+0xa50
warning: Could not find DWO CU drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.dwo(0x79060145b4b45807) referenced by CU at offset 0x394 [in module /usr/lib/debug/lib/modules/5.10.59-clang11debug/kernel/drivers/net/wireless/broadcom/brcm80211/brcmfmac/brcmfmac.ko]
0x277a4 is at ./include/linux/skbuff.h:2016.
2011	./include/linux/skbuff.h: No such file or directory.

Line 2016 of skbuff.h is __skb_insert in the next inline function:
static inline void __skb_queue_before(struct sk_buff_head *list,
                                      struct sk_buff *next,
                                      struct sk_buff *newsk)
{
        __skb_insert(newsk, next->prev, next, list);
}

Then:
(gdb) l *brcmf_sdio_dpc+0xa43
0x27797 is at drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c:2346.
2341	drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c: No such file or directory.

The line 2346 is in the function brcmf_sdio_sendfromq().

Best regards, Ryutaroh

Re: 5.10.58 UBSAN from brcmf_sdio_dpc+0xa50/0x128c [brcmfmac]

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 3729 bytes --]

On August 18, 2021 11:41:14 AM Ryutaroh Matsumoto 
<ryutaroh@ict.e.titech.ac.jp> wrote:

> Hi Arend, sorry for my late response, partly because compilation takes 10 hours
> on RPi4B...
> I did the same thing with version 5.10.59 and CLang 11 and got the same
> UBSAN.
>
>>> If you enable CONFIG_DEBUG_INFO in your kernel .config and recompile
>>> brcmfmac you can load the module in gdb:
>>> gdb> add-symbol-file brcmfmac.ko [address]
>>> gdb> l *brcmf_sdio_dpc+0xa50
>>> The [address] is not very important so just fill in a nice value. The
>>> 'l' command should provide the line number.
>>
>> Hi Ryutaroh,
>>
>> Meanwhile I did some digging in the brcmfmac driver and I think I
>> found the location in brcmf_sdio_sendfromq() where we do a
>> __skb_queue_tail(). So I looked at that and it does following:
>>
>> static inline void __skb_queue_tail(struct sk_buff_head *list,
>> struct sk_buff *newsk)
>> {
>> __skb_queue_before(list, (struct sk_buff *)list, newsk);
>> }
>>
>> Your report seems to be coming from the cast that is done here, which
>> is fine as long as sk_buff and sk_buff_head have the same members
>> 'next' and 'prev' at the start, which is true today and hopefully
>> forever ;-) I am inclined to say this is a false report.
>>
>> Can you please confirm the stack trace indeed points to
>> brcmf_sdio_sendfromq() in your report.
>
> Summary: I confirm that the stack trace indeed points to
> brcmf_sdio_sendfromq(). The detail follows (you don't have to read it if
> you believe in me :-)
> If kernel version 5.10.x or 5.13.x is compiled with gcc 10, I have never seen
> UBSAN. I wonder if CLang 11/12 tends to generate falsely positive UBSAN.

UBSAN seems to be wrong in this case...

>
> (gdb) add-symbol-file  
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/brcmfmac.ko 0x0
> add symbol table from file 
> "drivers/net/wireless/broadcom/brcm80211/brcmfmac/brcmfmac.ko" at
> .text_addr = 0x0
> (y or n) y
>
> (gdb)  l *brcmf_sdio_dpc+0xa50
> warning: Could not find DWO CU 
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.dwo(0x79060145b4b45807) 
> referenced by CU at offset 0x394 [in module 
> /usr/lib/debug/lib/modules/5.10.59-clang11debug/kernel/drivers/net/wireless/broadcom/brcm80211/brcmfmac/brcmfmac.ko]
> 0x277a4 is at ./include/linux/skbuff.h:2016.
> 2011 ./include/linux/skbuff.h: No such file or directory.
>
> Line 2016 of skbuff.h is __skb_insert in the next inline function:
> static inline void __skb_queue_before(struct sk_buff_head *list,
>                                      struct sk_buff *next,
>                                      struct sk_buff *newsk)
> {
>        __skb_insert(newsk, next->prev, next, list);
> }

... because the next pointer is same as list so it is sk_buff_head instance 
which is smaller than sk_buff, but using next->prev is ok given the 
structure definitions.

Thanks for the confirmation regarding the line number in brcmfmac source.

Regards,
Arend



-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]