• Maciej Żenczykowski's avatar
    usb: gadget: f_ncm: fix potential NULL ptr deref in ncm_bitrate() · c6ec9295
    Maciej Żenczykowski authored
    In Google internal bug 265639009 we've received an (as yet) unreproducible
    crash report from an aarch64 GKI 5.10.149-android13 running device.
    
    AFAICT the source code is at:
      https://android.googlesource.com/kernel/common/+/refs/tags/ASB-2022-12-05_13-5.10
    
    The call stack is:
      ncm_close() -> ncm_notify() -> ncm_do_notify()
    with the crash at:
      ncm_do_notify+0x98/0x270
    Code: 79000d0b b9000a6c f940012a f9400269 (b9405d4b)
    
    Which I believe disassembles to (I don't know ARM assembly, but it looks sane enough to me...):
    
      // halfword (16-bit) store presumably to event->wLength (at offset 6 of struct usb_cdc_notification)
      0B 0D 00 79    strh w11, [x8, #6]
    
      // word (32-bit) store presumably to req->Length (at offset 8 of struct usb_request)
      6C 0A 00 B9    str  w12, [x19, #8]
    
      // x10 (NULL) was read here from offset 0 of valid pointer x9
      // IMHO we're reading 'cdev->gadget' and getting NULL
      // gadget is indeed at offset 0 of struct usb_composite_dev
      2A 01 40 F9    ldr  x10, [x9]
    
      // loading req->buf pointer, which is at offset 0 of struct usb_request
      69 02 40 F9    ldr  x9, [x19]
    
      // x10 is null, crash, appears to be attempt to read cdev->gadget->max_speed
      4B 5D 40 B9    ldr  w11, [x10, #0x5c]
    
    which seems to line up with ncm_do_notify() case NCM_NOTIFY_SPEED code fragment:
    
      event->wLength = cpu_to_le16(8);
      req->length = NCM_STATUS_BYTECOUNT;
    
      /* SPEED_CHANGE data is up/down speeds in bits/sec */
      data = req->buf + sizeof *event;
      data[0] = cpu_to_le32(ncm_bitrate(cdev->gadget));
    
    My analysis of registers and NULL ptr deref crash offset
      (Unable to handle kernel NULL pointer dereference at virtual address 000000000000005c)
    heavily suggests that the crash is due to 'cdev->gadget' being NULL when executing:
      data[0] = cpu_to_le32(ncm_bitrate(cdev->gadget));
    which calls:
      ncm_bitrate(NULL)
    which then calls:
      gadget_is_superspeed(NULL)
    which reads
      ((struct usb_gadget *)NULL)->max_speed
    and hits a panic.
    
    AFAICT, if I'm counting right, the offset of max_speed is indeed 0x5C.
    (remember there's a GKI KABI reservation of 16 bytes in struct work_struct)
    
    It's not at all clear to me how this is all supposed to work...
    but returning 0 seems much better than panic-ing...
    
    Cc: Felipe Balbi <balbi@kernel.org>
    Cc: Lorenzo Colitti <lorenzo@google.com>
    Cc: Carlos Llamas <cmllamas@google.com>
    Cc: stable@vger.kernel.org
    Signed-off-by: default avatarMaciej Żenczykowski <maze@google.com>
    Cc: stable <stable@kernel.org>
    Link: https://lore.kernel.org/r/20230117131839.1138208-1-maze@google.comSigned-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    c6ec9295
f_ncm.c 46 KB