[BUG] eth_type_trans() can access stale data

[BUG] eth_type_trans() can access stale data

[Eric Dumazet]

Following code :

if (unlikely(skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF))
	return htons(ETH_P_802_3);

expects the additional bytes are in skb->head

I do not think all eth_type_trans() are ready for a pskb_may_pull()
(I am too lazy to perform a whole check)

Would following workaround be OK ?

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 8f032bae60ad..d95403098f09 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -194,7 +194,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
 	 *      won't work for fault tolerant netware but does for the rest.
 	 */
-	if (unlikely(skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF))
+	if (unlikely(skb_headlen(skb) >= 2 &&
+		     *(unsigned short *)(skb->data) == 0xFFFF))
 		return htons(ETH_P_802_3);
 
 	/*

Re: [BUG] eth_type_trans() can access stale data

[Ben Hutchings]

On Tue, 2014-01-14 at 15:52 -0800, Eric Dumazet wrote:
> Following code :
> 
> if (unlikely(skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF))
> 	return htons(ETH_P_802_3);
> 
> expects the additional bytes are in skb->head
>
> I do not think all eth_type_trans() are ready for a pskb_may_pull()
> (I am too lazy to perform a whole check)
> 
> Would following workaround be OK ?

It seems to be an improvement, as LLC packets will no longer falsely be
marked as Netware raw 802.3.  You don't fix errors in the other
direction, which should be more common, but the lack of bug reports
about that suggests that no-one is using the old Netware protocol any
more, or they're being very careful about which drivers they use.

Ben.

> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 8f032bae60ad..d95403098f09 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -194,7 +194,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>  	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
>  	 *      won't work for fault tolerant netware but does for the rest.
>  	 */
> -	if (unlikely(skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF))
> +	if (unlikely(skb_headlen(skb) >= 2 &&
> +		     *(unsigned short *)(skb->data) == 0xFFFF))
>  		return htons(ETH_P_802_3);
>  
>  	/*



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next] net: eth_type_trans() should use skb_header_pointer()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

eth_type_trans() can read uninitialized memory as drivers
do not necessarily pull more than 14 bytes in skb->head before
calling it.

As David suggested, we can use skb_header_pointer() to
fix this without breaking some drivers that might not expect
eth_type_trans() pulling 2 additional bytes.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
Since this bug is very old, I cooked the patch on net-next

 net/ethernet/eth.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 8f032bae60ad..5dc638cad2e1 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -156,7 +156,9 @@ EXPORT_SYMBOL(eth_rebuild_header);
  */
 __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 {
-	struct ethhdr *eth;
+	unsigned short _service_access_point;
+	const unsigned short *sap;
+	const struct ethhdr *eth;
 
 	skb->dev = dev;
 	skb_reset_mac_header(skb);
@@ -194,7 +196,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
 	 *      won't work for fault tolerant netware but does for the rest.
 	 */
-	if (unlikely(skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF))
+	sap = skb_header_pointer(skb, 0, sizeof(*sap), &_service_access_point);
+	if (sap && *sap == 0xFFFF)
 		return htons(ETH_P_802_3);
 
 	/*

Re: [PATCH net-next] net: eth_type_trans() should use skb_header_pointer()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 16 Jan 2014 15:03:31 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> eth_type_trans() can read uninitialized memory as drivers
> do not necessarily pull more than 14 bytes in skb->head before
> calling it.
> 
> As David suggested, we can use skb_header_pointer() to
> fix this without breaking some drivers that might not expect
> eth_type_trans() pulling 2 additional bytes.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> ---
> Since this bug is very old, I cooked the patch on net-next

Applied, thanks a lot Eric.

Re: [BUG] eth_type_trans() can access stale data

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 14 Jan 2014 15:52:21 -0800

> @@ -194,7 +194,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>  	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
>  	 *      won't work for fault tolerant netware but does for the rest.
>  	 */
> -	if (unlikely(skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF))
> +	if (unlikely(skb_headlen(skb) >= 2 &&
> +		     *(unsigned short *)(skb->data) == 0xFFFF))
>  		return htons(ETH_P_802_3);

I think using skb_header_pointer() will essentially have the same cost,
why not use it instead?

Thanks.

Re: [BUG] eth_type_trans() can access stale data

[Eric Dumazet]

On Wed, 2014-01-15 at 15:48 -0800, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 14 Jan 2014 15:52:21 -0800
> 
> > @@ -194,7 +194,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
> >  	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
> >  	 *      won't work for fault tolerant netware but does for the rest.
> >  	 */
> > -	if (unlikely(skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF))
> > +	if (unlikely(skb_headlen(skb) >= 2 &&
> > +		     *(unsigned short *)(skb->data) == 0xFFFF))
> >  		return htons(ETH_P_802_3);
> 
> I think using skb_header_pointer() will essentially have the same cost,
> why not use it instead?

Yes, we can try this, I'll send a patch, thanks.