Re: [linux-next:master] [kbuild] df8fc4e934: BUG:unable_to_handle_page_fault_for_address

Re: [linux-next:master] [kbuild] df8fc4e934: BUG:unable_to_handle_page_fault_for_address

[Kees Cook]

On Sun, Jun 11, 2023 at 09:41:30PM +0800, kernel test robot wrote:
> the issue we found below is by clang-15, but we confirmed clang-15 we use is
> commit 8dfdcc7b7b in llvm-project. it supports the flag already.

Interesting! Thanks for the report.

> [  228.605608][    C1] BUG: unable to handle page fault for address: 04090300
> [...]
> [ 228.608262][ C1] EIP: string (lib/vsprintf.c:644 lib/vsprintf.c:726) 
> [...]
> [ 228.608262][ C1] Call Trace:
> [ 228.608262][ C1]  <SOFTIRQ>
> [ 228.608262][ C1] vsnprintf (lib/vsprintf.c:2817) 
> [ 228.608262][ C1] vprintk_store (kernel/printk/printk.c:2191) 
> [ 228.608262][ C1] vprintk_emit (kernel/printk/printk.c:2288) 
> [ 228.608262][ C1] vprintk_default (kernel/printk/printk.c:2318) 
> [ 228.608262][ C1] vprintk (kernel/printk/printk_safe.c:50) 
> [ 228.608262][ C1] _printk (kernel/printk/printk.c:2331) 
> [ 228.608262][ C1] __ubsan_handle_out_of_bounds (lib/ubsan.c:209 lib/ubsan.c:343) 

This is a crash within the UBSAN handler! That's very unexpected.

> [ 228.608262][ C1] get_string (drivers/usb/gadget/composite.c:1314) 
> [ 228.608262][ C1] composite_setup (drivers/usb/gadget/composite.c:1871) 
> [ 228.608262][ C1] dummy_timer (drivers/usb/gadget/udc/dummy_hcd.c:?) 

And the out-of-bounds condition got triggered in dummy_hcd.

I also see this is happening in SOFTIRQ context. I wonder if there is a
problem with UBSAN vs SOFTIRQ. Again, I'd find that surprising...

I'll see what I can find...

-- 
Kees Cook

Re: [linux-next:master] [kbuild] df8fc4e934: BUG:unable_to_handle_page_fault_for_address

[Kees Cook]

On Tue, Jun 13, 2023 at 01:46:54PM -0700, Kees Cook wrote:
> On Sun, Jun 11, 2023 at 09:41:30PM +0800, kernel test robot wrote:
> > the issue we found below is by clang-15, but we confirmed clang-15 we use is
> > commit 8dfdcc7b7b in llvm-project. it supports the flag already.
> 
> Interesting! Thanks for the report.
> 
> > [  228.605608][    C1] BUG: unable to handle page fault for address: 04090300
> > [...]
> > [ 228.608262][ C1] EIP: string (lib/vsprintf.c:644 lib/vsprintf.c:726) 
> > [...]
> > [ 228.608262][ C1] Call Trace:
> > [ 228.608262][ C1]  <SOFTIRQ>
> > [ 228.608262][ C1] vsnprintf (lib/vsprintf.c:2817) 
> > [ 228.608262][ C1] vprintk_store (kernel/printk/printk.c:2191) 
> > [ 228.608262][ C1] vprintk_emit (kernel/printk/printk.c:2288) 
> > [ 228.608262][ C1] vprintk_default (kernel/printk/printk.c:2318) 
> > [ 228.608262][ C1] vprintk (kernel/printk/printk_safe.c:50) 
> > [ 228.608262][ C1] _printk (kernel/printk/printk.c:2331) 
> > [ 228.608262][ C1] __ubsan_handle_out_of_bounds (lib/ubsan.c:209 lib/ubsan.c:343) 
> 
> This is a crash within the UBSAN handler! That's very unexpected.

I still don't understand this. Is printk() not allowed in SOFTIRQ?

> > [ 228.608262][ C1] get_string (drivers/usb/gadget/composite.c:1314) 
> > [ 228.608262][ C1] composite_setup (drivers/usb/gadget/composite.c:1871) 
> > [ 228.608262][ C1] dummy_timer (drivers/usb/gadget/udc/dummy_hcd.c:?) 
> 
> And the out-of-bounds condition got triggered in dummy_hcd.

I found this. I'm surprised we didn't trip over it earlier!

/* USB_DT_STRING: String descriptor */
struct usb_string_descriptor {
        __u8  bLength;
        __u8  bDescriptorType;

        __le16 wData[1];                /* UTF-16LE encoded */
} __attribute__ ((packed));

$ git grep 'struct usb_string_descriptor'
drivers/usb/early/xhci-dbc.c:   struct usb_string_descriptor *s_desc;
drivers/usb/early/xhci-dbc.c:   s_desc                  = (struct usb_string_descriptor *)strings->serial;
drivers/usb/early/xhci-dbc.c:   s_desc                  = (struct usb_string_descriptor *)strings->product;
drivers/usb/early/xhci-dbc.c:   s_desc                  = (struct usb_string_descriptor *)strings->manufacturer;
drivers/usb/gadget/composite.c:         struct usb_string_descriptor    *s = buf;
drivers/usb/gadget/udc/aspeed-vhub/hub.c:       struct usb_string_descriptor *sdesc = buf;
drivers/usb/host/xhci-dbgcap.c: struct usb_string_descriptor    *s_desc;
drivers/usb/host/xhci-dbgcap.c: s_desc = (struct usb_string_descriptor *)strings->serial;
drivers/usb/host/xhci-dbgcap.c: s_desc = (struct usb_string_descriptor *)strings->product;
drivers/usb/host/xhci-dbgcap.c: s_desc = (struct usb_string_descriptor *)strings->manufacturer;
include/uapi/linux/usb/ch9.h:struct usb_string_descriptor {

Looking at each use, none are using sizeof() on the struct, so it should
be a trivial replacement. I'll send a patch.

-Kees

-- 
Kees Cook