@@ -224,9 +224,9 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
struct usb_cdc_ncm_ndp16 *ncm0;
struct usb_cdc_ncm_dpe16 *dpe;
struct ipheth_device *dev;
+ u16 dg_idx, dg_len;
int retval = -EINVAL;
char *buf;
- int len;
dev = urb->context;
@@ -238,39 +238,43 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
ncmh = urb->transfer_buffer;
if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) ||
/* On iOS, NDP16 directly follows NTH16 */
- ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16))) {
- dev->net->stats.rx_errors++;
- return retval;
- }
+ ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16)))
+ goto rx_error;
ncm0 = urb->transfer_buffer + sizeof(struct usb_cdc_ncm_nth16);
- if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN)) {
- dev->net->stats.rx_errors++;
- return retval;
- }
+ if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN))
+ goto rx_error;
dpe = ncm0->dpe16;
- while (le16_to_cpu(dpe->wDatagramIndex) != 0 &&
- le16_to_cpu(dpe->wDatagramLength) != 0) {
- if (le16_to_cpu(dpe->wDatagramIndex) < IPHETH_NCM_HEADER_SIZE ||
- le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
- le16_to_cpu(dpe->wDatagramLength) > urb->actual_length -
- le16_to_cpu(dpe->wDatagramIndex)) {
+ while (true) {
+ dg_idx = le16_to_cpu(dpe->wDatagramIndex);
+ dg_len = le16_to_cpu(dpe->wDatagramLength);
+
+ /* Null DPE must be present after last datagram pointer entry
+ * (3.3.1 USB CDC NCM spec v1.0)
+ */
+ if (dg_idx == 0 && dg_len == 0)
+ return 0;
+
+ if (dg_idx < IPHETH_NCM_HEADER_SIZE ||
+ dg_idx >= urb->actual_length ||
+ dg_len > urb->actual_length - dg_idx) {
dev->net->stats.rx_length_errors++;
return retval;
}
- buf = urb->transfer_buffer + le16_to_cpu(dpe->wDatagramIndex);
- len = le16_to_cpu(dpe->wDatagramLength);
+ buf = urb->transfer_buffer + dg_idx;
- retval = ipheth_consume_skb(buf, len, dev);
+ retval = ipheth_consume_skb(buf, dg_len, dev);
if (retval != 0)
return retval;
dpe++;
}
- return 0;
+rx_error:
+ dev->net->stats.rx_errors++;
+ return retval;
}
static void ipheth_rcvbulk_callback(struct urb *urb)
Introduce an rx_error label to reduce repetitions in the header signature checks. Store wDatagramIndex and wDatagramLength after endianness conversion to avoid repeated le16_to_cpu() calls. Rewrite the loop to return on a null trailing DPE, which is required by the CDC NCM spec. In case it is missing, fall through to rx_error. This change does not fix any particular issue. Its purpose is to simplify a subsequent commit that fixes a potential OoB read by limiting the maximum amount of processed DPEs. Fix an out-of-bounds DPE read, limit the number of processed DPEs to the amount that fits into the fixed-size NDP16 header. Signed-off-by: Foster Snowhill <forst@pen.gy> --- v4: Split from "usbnet: ipheth: refactor NCM datagram loop, fix DPE OoB read" in v3. This commit is responsible for the code refactor, while keeping the behaviour the same. v3: https://lore.kernel.org/netdev/20241123235432.821220-5-forst@pen.gy/ Split out from a monolithic patch in v2 as an atomic change. v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/ No code changes. Update commit message to further clarify that `ipheth` is not and does not aim to be a complete or spec-compliant CDC NCM implementation. v1: https://lore.kernel.org/netdev/20240907230108.978355-1-forst@pen.gy/ --- drivers/net/usb/ipheth.c | 42 ++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 19 deletions(-)