linux.git/drivers/usb/gadget/function/f_ncm.c, branch v6.5

usb: gadget: f_ncm: fix potential NULL ptr deref in ncm_bitrate()

2023-01-17T16:10:34+00:00

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 
Cc: Lorenzo Colitti 
Cc: Carlos Llamas 
Cc: stable@vger.kernel.org
Signed-off-by: Maciej Żenczykowski 
Cc: stable 
Link: https://lore.kernel.org/r/20230117131839.1138208-1-maze@google.com
Signed-off-by: Greg Kroah-Hartman

usb: gadget: f_ncm: noop - remove INIT_NDP{16,32}_OPTS macros

2022-08-31T07:07:52+00:00

these are only used in one place, a few lines lower

Cc: Brooke Basile 
Cc: "Bryan O'Donoghue" 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Link: https://lore.kernel.org/r/20220821145745.122587-1-zenczykowski@gmail.com
Signed-off-by: Greg Kroah-Hartman

usb: gadget: f_ncm: ncm_wrap_ntb - move var definitions into if statement

2021-07-21T08:04:19+00:00

Since they're only used if there's an skb.

Cc: Brooke Basile 
Cc: "Bryan O'Donoghue" 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Link: https://lore.kernel.org/r/20210701114834.884597-5-zenczykowski@gmail.com
Signed-off-by: Greg Kroah-Hartman

usb: gadget: f_ncm: remove spurious if statement

2021-07-21T08:04:19+00:00

the current logic is:

  struct sk_buff  *skb2 = NULL;
  ...

  if (!skb && !ncm->skb_tx_data)
    return NULL;

  if (skb) {
    ...
  } else if (ncm->skb_tx_data)
    ...
  }

  return skb2;

Which means that first if statement is simply not needed.

Cc: Brooke Basile 
Cc: "Bryan O'Donoghue" 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Link: https://lore.kernel.org/r/20210701114834.884597-4-zenczykowski@gmail.com
Signed-off-by: Greg Kroah-Hartman

usb: gadget: f_ncm: remove check for NULL skb_tx_data in timer function

2021-07-21T08:04:19+00:00

This condition is already checked for in ncm_wrap_ntb(),
except that that check is done with eth_dev->lock held
(it is grabbed by eth_start_xmit).

It's best to not be reaching into ncm struct without locks held.

Cc: Brooke Basile 
Cc: "Bryan O'Donoghue" 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Link: https://lore.kernel.org/r/20210701114834.884597-3-zenczykowski@gmail.com
Signed-off-by: Greg Kroah-Hartman

usb: gadget: f_ncm: remove spurious boolean timer_stopping

2021-07-21T08:04:19+00:00

It is equivalent to ncm->netdev being NULL.

Cc: Brooke Basile 
Cc: "Bryan O'Donoghue" 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Link: https://lore.kernel.org/r/20210701114834.884597-2-zenczykowski@gmail.com
Signed-off-by: Greg Kroah-Hartman

usb: gadget: f_ncm: remove timer_force_tx field

2021-07-21T08:04:19+00:00

It is simply not needed.  This field is equivalent to skb being NULL.

Currently with the boolean set to true we call:
  ncm->netdev->netdev_ops->ndo_start_xmit(NULL, ncm->netdev);
which calls u_ether's:
  eth_start_xmit(NULL, ...)
which then calls:
  skb = dev->wrap(dev->port_usb, NULL);
which calls back into f_ncm's:
  ncm_wrap_ntb(..., NULL)

Cc: Brooke Basile 
Cc: "Bryan O'Donoghue" 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Link: https://lore.kernel.org/r/20210701114834.884597-1-zenczykowski@gmail.com
Signed-off-by: Greg Kroah-Hartman

usb: f_ncm: only first packet of aggregate needs to start timer

2021-06-09T08:34:18+00:00

The reasoning for this change is that if we already had
a packet pending, then we also already had a pending timer,
and as such there is no need to reschedule it.

This also prevents packets getting delayed 60 ms worst case
under a tiny packet every 290us transmit load, by keeping the
timeout always relative to the first queued up packet.
(300us delay * 16KB max aggregation / 80 byte packet =~ 60 ms)

As such the first packet is now at most delayed by 300us.

Under low transmit load, this will simply result in us sending
a shorter aggregate, as originally intended.

This patch has the benefit of greatly reducing (by ~10 factor
with 1500 byte frames aggregated into 16 kiB) the number of
(potentially pretty costly) updates to the hrtimer.

Cc: Brooke Basile 
Cc: Bryan O'Donoghue 
Cc: Felipe Balbi 
Cc: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Link: https://lore.kernel.org/r/20210608085438.813960-1-zenczykowski@gmail.com
Cc: stable 
Signed-off-by: Greg Kroah-Hartman

USB: f_ncm: ncm_bitrate (speed) is unsigned

2021-06-09T08:26:43+00:00

[  190.544755] configfs-gadget gadget: notify speed -44967296

This is because 4250000000 - 2**32 is -44967296.

Fixes: 9f6ce4240a2b ("usb: gadget: f_ncm.c added")
Cc: Brooke Basile 
Cc: Bryan O'Donoghue 
Cc: Felipe Balbi 
Cc: Lorenzo Colitti 
Cc: Yauheni Kaliuta 
Cc: Linux USB Mailing List 
Acked-By: Lorenzo Colitti 
Signed-off-by: Maciej Żenczykowski 
Cc: stable 
Link: https://lore.kernel.org/r/20210608005344.3762668-1-zenczykowski@gmail.com
Signed-off-by: Greg Kroah-Hartman

usb: gadget: f_ncm: allow using NCM in SuperSpeed Plus gadgets.

2020-10-02T06:57:41+00:00

Currently, enabling f_ncm at SuperSpeed Plus speeds results in an
oops in config_ep_by_speed because ncm_set_alt passes in NULL
ssp_descriptors. Fix this by re-using the SuperSpeed descriptors.
This is safe because usb_assign_descriptors calls
usb_copy_descriptors.

Tested: enabled f_ncm on a dwc3 gadget and 10Gbps link, ran iperf
Reviewed-by: Maciej Żenczykowski 
Signed-off-by: Lorenzo Colitti 
Signed-off-by: Felipe Balbi