[PATCH net] net: qualcomm: rmnet: validate MAP frame length before ingress parsing

[PATCH net] net: qualcomm: rmnet: validate MAP frame length before ingress parsing

[Xiang Mei]

__rmnet_map_ingress_handler() casts skb->data to a struct rmnet_map_header
and trusts the on-wire pkt_len field without verifying that the skb
actually contains the bytes it is about to dereference. When ingress
deaggregation is disabled (RMNET_FLAGS_INGRESS_DEAGGREGATION cleared), the
skb is handed to __rmnet_map_ingress_handler() directly, bypassing the
length check that rmnet_map_deaggregate() performs.

Three distinct reads past skb->len are reachable, so three guards are
needed:

 - The MAP header (flags, mux_id, pkt_len) is read on entry, before either
   csum branch and even on the command and non-csum paths, so a runt frame
   faults here regardless of data_format. Guard the header first.
 - On the MAPv5 next-header path, rmnet_map_process_next_hdr_packet() reads
   the v5 csum header at (skb->data + sizeof(map_header)); the entry guard
   only covers the header, so this needs its own check.
 - On the MAPv4 path, rmnet_map_checksum_downlink_packet() reads the csum
   trailer at (skb->data + len); guard it under the same CKSUMV4 condition
   the read uses, or valid non-csum frames would be dropped.

Each guard uses pskb_may_pull() before its read. The MAPv4 check uses
ntohs(pkt_len) (== payload + pad) directly rather than the derived len, so
it is unaffected by the u16 underflow in len when pad > pkt_len. Well-formed
frames always carry the header/trailer they declare, so only malformed
packets are dropped; this mirrors the length check rmnet_map_deaggregate()
already performs on the deaggregation path.

  BUG: KASAN: slab-out-of-bounds in rmnet_map_checksum_downlink_packet
  Read of size 1 at addr ffff88801118ed00 by task exploit/147
  Call Trace:
   ...
   rmnet_map_checksum_downlink_packet (drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:413)
   __rmnet_map_ingress_handler (drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:96)
   rmnet_rx_handler (drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:129)
   __netif_receive_skb_core.constprop.0 (net/core/dev.c:6089)
   netif_receive_skb (net/core/dev.c:6460)
   tun_get_user (drivers/net/tun.c:1955)
   tun_chr_write_iter (drivers/net/tun.c:2001)
   vfs_write (fs/read_write.c:688)
   ksys_write (fs/read_write.c:740)
   do_syscall_64 (arch/x86/entry/syscall_64.c:94)
   ...

Fixes: ceed73a2cf4a ("drivers: net: ethernet: qualcomm: rmnet: Initial implementation")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 9f3479500f85..83d011ed5942 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -61,6 +61,9 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 	u16 len, pad;
 	u8 mux_id;
 
+	if (!pskb_may_pull(skb, sizeof(*map_header)))
+		goto free_skb;
+
 	if (map_header->flags & MAP_CMD_FLAG) {
 		/* Packet contains a MAP command (not data) */
 		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
@@ -84,11 +87,19 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 
 	if ((port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
 	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
+		if (!pskb_may_pull(skb, sizeof(*map_header) +
+				   sizeof(struct rmnet_map_v5_csum_header)))
+			goto free_skb;
 		if (rmnet_map_process_next_hdr_packet(skb, len))
 			goto free_skb;
 		skb_pull(skb, sizeof(*map_header));
 		rmnet_set_skb_proto(skb);
 	} else {
+		if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4 &&
+		    !pskb_may_pull(skb, sizeof(*map_header) +
+				   ntohs(map_header->pkt_len) +
+				   sizeof(struct rmnet_map_dl_csum_trailer)))
+			goto free_skb;
 		/* Subtract MAP header */
 		skb_pull(skb, sizeof(*map_header));
 		rmnet_set_skb_proto(skb);
-- 
2.43.0

RE: [PATCH net] net: qualcomm: rmnet: validate MAP frame length before ingress parsing

[subash.a.kasiviswanathan]

> -----Original Message-----
> From: Xiang Mei <xmei5@asu.edu>
> Each guard uses pskb_may_pull() before its read. The MAPv4 check uses
> ntohs(pkt_len) (== payload + pad) directly rather than the derived len, so
it is
> unaffected by the u16 underflow in len when pad > pkt_len. Well-formed
> frames always carry the header/trailer they declare, so only malformed
> packets are dropped; this mirrors the length check rmnet_map_deaggregate()
> already performs on the deaggregation path.
> 
> @@ -61,6 +61,9 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
>  	u16 len, pad;
>  	u8 mux_id;
> 
> +	if (!pskb_may_pull(skb, sizeof(*map_header)))
> +		goto free_skb;
> +
>  	if (map_header->flags & MAP_CMD_FLAG) {
>  		/* Packet contains a MAP command (not data) */
>  		if (port->data_format &
> RMNET_FLAGS_INGRESS_MAP_COMMANDS) @@ -84,11 +87,19 @@
> __rmnet_map_ingress_handler(struct sk_buff *skb,
> 
>  	if ((port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5)
> &&
>  	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
> +		if (!pskb_may_pull(skb, sizeof(*map_header) +
> +				   sizeof(struct
> rmnet_map_v5_csum_header)))
> +			goto free_skb;
>  		if (rmnet_map_process_next_hdr_packet(skb, len))
>  			goto free_skb;
>  		skb_pull(skb, sizeof(*map_header));
>  		rmnet_set_skb_proto(skb);
>  	} else {
> +		if (port->data_format &
> RMNET_FLAGS_INGRESS_MAP_CKSUMV4 &&
> +		    !pskb_may_pull(skb, sizeof(*map_header) +
> +				   ntohs(map_header->pkt_len) +
> +				   sizeof(struct
rmnet_map_dl_csum_trailer)))
> +			goto free_skb;
>  		/* Subtract MAP header */
>  		skb_pull(skb, sizeof(*map_header));
>  		rmnet_set_skb_proto(skb);
> --
> 2.43.0

The patch seems fine at a high level. However, it ends up adding duplicate
checks for the deagg path which is the commonly used path.
Perhaps you can try something similar to the following which adds all the
relevant checks of the deagg path to the no agg path as well.

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 9f3479500f85..086874b673c6 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -126,7 +126,8 @@ rmnet_map_ingress_handler(struct sk_buff *skb,
 
                consume_skb(skb);
        } else {
-               __rmnet_map_ingress_handler(skb, port);
+               if (rmnet_map_validate_packet_len(skb, port))
+                       __rmnet_map_ingress_handler(skb, port);
        }
 }
 
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index b70284095568..46495b7966f3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -59,5 +59,7 @@ void rmnet_map_tx_aggregate_init(struct rmnet_port *port);
 void rmnet_map_tx_aggregate_exit(struct rmnet_port *port);
 void rmnet_map_update_ul_agg_config(struct rmnet_port *port, u32 size,
                                    u32 count, u32 time);
+u32 rmnet_map_validate_packet_len(struct sk_buff *skb,
+                                 struct rmnet_port *port);
 
 #endif /* _RMNET_MAP_H_ */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 8b4640c5d61e..1729b73249f7 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -333,24 +333,16 @@ struct rmnet_map_header
*rmnet_map_add_map_header(struct sk_buff *skb,
        return map_header;
 }
 
-/* Deaggregates a single packet
- * A whole new buffer is allocated for each portion of an aggregated frame.
- * Caller should keep calling deaggregate() on the source skb until 0 is
- * returned, indicating that there are no more packets to deaggregate.
Caller
- * is responsible for freeing the original skb.
- */
-struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
-                                     struct rmnet_port *port)
-{
+u32 rmnet_map_validate_packet_len(struct sk_buff *skb,
+                                 struct rmnet_port *port) {
        struct rmnet_map_v5_csum_header *next_hdr = NULL;
        struct rmnet_map_header *maph;
        void *data = skb->data;
-       struct sk_buff *skbn;
        u8 nexthdr_type;
        u32 packet_len;
 
        if (skb->len == 0)
-               return NULL;
+               goto err1;
 
        maph = (struct rmnet_map_header *)skb->data;
        packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
@@ -364,24 +356,48 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff
*skb,
                                next_hdr = data + sizeof(*maph);
                        else
                                /* Mapv5 data pkt without csum hdr is
invalid */
-                               return NULL;
+                               goto err1;
                }
        }
 
        if (((int)skb->len - (int)packet_len) < 0)
-               return NULL;
+               goto err1;
 
        /* Some hardware can send us empty frames. Catch them */
        if (!maph->pkt_len)
-               return NULL;
+               goto err1;
 
        if (next_hdr) {
                nexthdr_type = u8_get_bits(next_hdr->header_info,
                                           MAPV5_HDRINFO_HDR_TYPE_FMASK);
                if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
-                       return NULL;
+                       goto err1;
        }
 
+       goto err0;
+
+err1:
+       packet_len = 0;
+err0:
+       return packet_len;
+}
+
+/* Deaggregates a single packet
+ * A whole new buffer is allocated for each portion of an aggregated frame.
+ * Caller should keep calling deaggregate() on the source skb until 0 is
+ * returned, indicating that there are no more packets to deaggregate.
Caller
+ * is responsible for freeing the original skb.
+ */
+struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
+                                     struct rmnet_port *port)
+{
+       struct sk_buff *skbn;
+       u32 packet_len;
+
+       packet_len = rmnet_map_validate_packet_len(skb, port);
+       if (!packet_len)
+               return NULL;
+
        skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
        if (!skbn)
                return NULL;