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

[<subash.a.kasiviswanathan@oss.qualcomm.com>]

> -----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;