New: LLC PDU is dropped if skb is not linear

Bugme-new Andrew Morton 2 participants 2 messages Apr 11, 2011 – Apr 12, 2011 1 message not in the archive

Subsystems (3)

Networking Network drivers LLC (802.2)

lore

Share & tools

lore

Andrew Morton

author

2011-04-11 23:48 UTC #1 lore raw

In reply to a message not in the archive

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 34 lines · On Fri, 8 Apr 2011 09:52:34 GMT [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=32872

           Summary: LLC PDU is dropped if skb is not linear
           Product: Networking
           Version: 2.5
    Kernel Version: 2.6.32
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Other
        AssignedTo: [email protected]
        ReportedBy: [email protected]
        Regression: No

If skb consists of several fragments this check fails net/llc/llc_input.c:
=============================================
llc_fixup_skb()
...

    pdu = (struct llc_pdu_un *)skb->data;
...
    __be16 pdulen = eth_hdr(skb)->h_proto;
    s32 data_size = ntohs(pdulen) - llc_len;

    if (data_size < 0 ||
        ((skb_tail_pointer(skb) -
          (u8 *)pdu) - llc_len) < data_size)
        return 0;

=============================================

Quoted 8 lines

and packet is silently dropped.
This breaks GVRP protocol if received packet is large ( > 512 bytes on my system) and contains a Leave All message. Since Leave All is missed, corresponding JoinIns are not sent and switch unregisters VLANs from port.
Attached patch seems to resolve this issue, but I don't know if it's a correct solution.

net/llc/llc_input.c renamed from net/llc/llc_input.c.orig +5 -0

@@ -105,6 +105,11 @@
 	if (unlikely(!pskb_may_pull(skb, sizeof(*pdu))))
 		return 0;
 
+	if (skb->data_len != 0){
+	    if (unlikely(skb_linearize(skb)))
+		return 0;
+	}
+
 	pdu = (struct llc_pdu_un *)skb->data;
 	if ((pdu->ctrl_1 & LLC_PDU_TYPE_MASK) == LLC_PDU_TYPE_U)
 		llc_len = 1;

2.6.32 is a pretty old kernel - we'll need to verify if current kernels have the same problem.

Please don't send patches via bugzilla - it causes lots of problems with our usual patch management and review processes. It's preferred that patches be sent via email as per Documentation/SubmittingPatches, and that they include a Signed-off-by:, as described in that file.

Thanks.

David Miller

maintainer

2011-04-12 01:57 UTC #2 lore raw

From: Andrew Morton <[email protected]>
Date: Mon, 11 Apr 2011 16:48:12 -0700

Quoted 16 lines

net/llc/llc_input.c renamed from net/llc/llc_input.c.orig +5 -0

@@ -105,6 +105,11 @@
 	if (unlikely(!pskb_may_pull(skb, sizeof(*pdu))))
 		return 0;
 
+	if (skb->data_len != 0){
+	    if (unlikely(skb_linearize(skb)))
+		return 0;
+	}
+
 	pdu = (struct llc_pdu_un *)skb->data;
 	if ((pdu->ctrl_1 & LLC_PDU_TYPE_MASK) == LLC_PDU_TYPE_U)
 		llc_len = 1;

Quoted 7 lines

2.6.32 is a pretty old kernel - we'll need to verify if current kernels have the same problem.
Please don't send patches via bugzilla - it causes lots of problems with our usual patch management and review processes. It's preferred that patches be sent via email as per Documentation/SubmittingPatches, and that they include a Signed-off-by:, as described in that file.

The skb_tail_pointer() check in llc_fixup_skb() is beyond wonky and honestly the source of the problems here.

I'd suggest instead:

net/llc/llc_input.c +1 -2

index 058f1e9..9032421 100644
@@ -121,8 +121,7 @@ static inline int llc_fixup_skb(struct sk_buff *skb)
 		s32 data_size = ntohs(pdulen) - llc_len;
 
 		if (data_size < 0 ||
-		    ((skb_tail_pointer(skb) -
-		      (u8 *)pdu) - llc_len) < data_size)
+		    !pskb_may_pull(skb, data_size))
 			return 0;
 		if (unlikely(pskb_trim_rcsum(skb, data_size)))
 			return 0;