| Message ID | 20230830100418.1952143-1-ricardo.canuelo@collabora.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f74a7afc224acd5e922c7a2e52244d891bbe44ee |
| Headers | show |
| Series | usb: hub: Guard against accesses to uninitialized BOS descriptors | expand |
On Wed, Aug 30, 2023 at 01:01:29PM +0200, Ricardo Cañuelo wrote: > Re-sending the email, the previous one was bounced, sorry for the noise. > > Hi Greg, thanks for reviewing. > > On mié, ago 30 2023 at 12:13:44, Greg KH <gregkh@linuxfoundation.org> wrote: > > Did you mean to loose the indentation here? > > This output has probably gone through some processing before I got it, > so that's likely the cause. I don't have access to the original output, > sorry. If I need to fix something for a v2 I can try to re-format it to > add the indentation back in. > > > What commit id does this fix? > > This isn't linked to a particular single commit, it fixes the same > problem in different parts of the code, introduced by different commits > over time. The starting point for the fix was the crash output in the > commit message but, for additional context, these are the original > commits that introduced the unguarded struct accesses: > > 0cdd49a1d1a4 ("usb: Support USB 3.1 extended port status request") > 51e0a0120661 ("USB: Calculate USB 3.0 exit latencies for LPM.") > 25cd2882e2fc ("usb/xhci: Change how we indicate a host supports Link PM.") > 0299809be415 ("usb: core: Track SuperSpeed Plus GenXxY") > ae8963adb4ad ("usb: Don't enable LPM if the exit latency is zero.") As all of these are older than the oldest supported LTS kernel, we should be fine in taking it all the way back to 4.14.y then. I'll queue this up after 6.6-rc1 is out, thanks. greg k-h
Hi, On mié, ago 30 2023 at 13:12:03, Greg KH <gregkh@linuxfoundation.org> wrote: > As all of these are older than the oldest supported LTS kernel, we > should be fine in taking it all the way back to 4.14.y then. > > I'll queue this up after 6.6-rc1 is out, thanks. Gentle ping, what's the merge status of this patch? I haven't seen it on -next yet. Cheers, Ricardo
On Fri, Sep 22, 2023 at 07:40:32AM +0200, Ricardo Cañuelo wrote: > Hi, > > On mié, ago 30 2023 at 13:12:03, Greg KH <gregkh@linuxfoundation.org> wrote: > > As all of these are older than the oldest supported LTS kernel, we > > should be fine in taking it all the way back to 4.14.y then. > > > > I'll queue this up after 6.6-rc1 is out, thanks. > > Gentle ping, what's the merge status of this patch? I haven't seen it on > -next yet. Please relax, it was the merge window and then I have been traveling for conferences. If you wish to have patches applied sooner, please help out in reviewing the other outstanding patches on the mailing list to provide help to reduce the overall load. thanks, greg k-h
Hi Greg, On vie, sep 22 2023 at 11:12:31, Greg KH <gregkh@linuxfoundation.org> wrote: > Please relax, it was the merge window and then I have been traveling for > conferences. If you wish to have patches applied sooner, please help > out in reviewing the other outstanding patches on the mailing list to > provide help to reduce the overall load. Thanks, and apologies if I sounded pushy, I'm simply going through patches and trying to stay up to date with their status, no hurry. I appreciate your effort. Cheers, Ricardo
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index a739403a9e45..7e2d092df893 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -151,6 +151,10 @@ int usb_device_supports_lpm(struct usb_device *udev) if (udev->quirks & USB_QUIRK_NO_LPM) return 0; + /* Skip if the device BOS descriptor couldn't be read */ + if (!udev->bos) + return 0; + /* USB 2.1 (and greater) devices indicate LPM support through * their USB 2.0 Extended Capabilities BOS descriptor. */ @@ -327,6 +331,10 @@ static void usb_set_lpm_parameters(struct usb_device *udev) if (!udev->lpm_capable || udev->speed < USB_SPEED_SUPER) return; + /* Skip if the device BOS descriptor couldn't be read */ + if (!udev->bos) + return; + hub = usb_hub_to_struct_hub(udev->parent); /* It doesn't take time to transition the roothub into U0, since it * doesn't have an upstream link. @@ -2715,13 +2723,17 @@ int usb_authorize_device(struct usb_device *usb_dev) static enum usb_ssp_rate get_port_ssp_rate(struct usb_device *hdev, u32 ext_portstatus) { - struct usb_ssp_cap_descriptor *ssp_cap = hdev->bos->ssp_cap; + struct usb_ssp_cap_descriptor *ssp_cap; u32 attr; u8 speed_id; u8 ssac; u8 lanes; int i; + if (!hdev->bos) + goto out; + + ssp_cap = hdev->bos->ssp_cap; if (!ssp_cap) goto out; @@ -4239,8 +4251,15 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev, enum usb3_link_state state) { int timeout; - __u8 u1_mel = udev->bos->ss_cap->bU1devExitLat; - __le16 u2_mel = udev->bos->ss_cap->bU2DevExitLat; + __u8 u1_mel; + __le16 u2_mel; + + /* Skip if the device BOS descriptor couldn't be read */ + if (!udev->bos) + return; + + u1_mel = udev->bos->ss_cap->bU1devExitLat; + u2_mel = udev->bos->ss_cap->bU2DevExitLat; /* If the device says it doesn't have *any* exit latency to come out of * U1 or U2, it's probably lying. Assume it doesn't implement that link diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h index 37897afd1b64..d44dd7f6623e 100644 --- a/drivers/usb/core/hub.h +++ b/drivers/usb/core/hub.h @@ -153,7 +153,7 @@ static inline int hub_is_superspeedplus(struct usb_device *hdev) { return (hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS && le16_to_cpu(hdev->descriptor.bcdUSB) >= 0x0310 && - hdev->bos->ssp_cap); + hdev->bos && hdev->bos->ssp_cap); } static inline unsigned hub_power_on_good_delay(struct usb_hub *hub)
Many functions in drivers/usb/core/hub.c and drivers/usb/core/hub.h access fields inside udev->bos without checking if it was allocated and initialized. If usb_get_bos_descriptor() fails for whatever reason, udev->bos will be NULL and those accesses will result in a crash: BUG: kernel NULL pointer dereference, address: 0000000000000018 PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP NOPTI CPU: 5 PID: 17818 Comm: kworker/5:1 Tainted: G W 5.15.108-18910-gab0e1cb584e1 #1 <HASH:1f9e 1> Hardware name: Google Kindred/Kindred, BIOS Google_Kindred.12672.413.0 02/03/2021 Workqueue: usb_hub_wq hub_event RIP: 0010:hub_port_reset+0x193/0x788 Code: 89 f7 e8 20 f7 15 00 48 8b 43 08 80 b8 96 03 00 00 03 75 36 0f b7 88 92 03 00 00 81 f9 10 03 00 00 72 27 48 8b 80 a8 03 00 00 <48> 83 78 18 00 74 19 48 89 df 48 8b 75 b0 ba 02 00 00 00 4c 89 e9 RSP: 0018:ffffab740c53fcf8 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffffa1bc5f678000 RCX: 0000000000000310 RDX: fffffffffffffdff RSI: 0000000000000286 RDI: ffffa1be9655b840 RBP: ffffab740c53fd70 R08: 00001b7d5edaa20c R09: ffffffffb005e060 R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000 R13: ffffab740c53fd3e R14: 0000000000000032 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffffa1be96540000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000018 CR3: 000000022e80c005 CR4: 00000000003706e0 Call Trace: hub_event+0x73f/0x156e ? hub_activate+0x5b7/0x68f process_one_work+0x1a2/0x487 worker_thread+0x11a/0x288 kthread+0x13a/0x152 ? process_one_work+0x487/0x487 ? kthread_associate_blkcg+0x70/0x70 ret_from_fork+0x1f/0x30 Fall back to a default behavior if the BOS descriptor isn't accessible and skip all the functionalities that depend on it: LPM support checks, Super Speed capabilitiy checks, U1/U2 states setup. Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com> Cc: stable <stable@vger.kernel.org> --- drivers/usb/core/hub.c | 25 ++++++++++++++++++++++--- drivers/usb/core/hub.h | 2 +- 2 files changed, 23 insertions(+), 4 deletions(-)