[PATCH] tap: fix uses of l3_len in tap4_handler()

[PATCH] tap: fix uses of l3_len in tap4_handler()

[Stas Sergeev]

l3_len was calculated from the ethernet frame size, and it
was assumed to be equal to the length stored in an IP packet.
But if the ethernet frame is padded, then l3_len calculated
that way can only be used as a bound check to validate the
length stored in an IP header. It should not be used for
calculating the l4_len.

This patch makes sure the small padded ethernet frames are
properly processed, by trusting the length stored in an IP
header.

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Stefano Brivio <sbrivio@redhat.com>
---
 tap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tap.c b/tap.c
index ee79be0..8d7859c 100644
--- a/tap.c
+++ b/tap.c
@@ -615,7 +615,7 @@ resume:
 			continue;
 
 		hlen = iph->ihl * 4UL;
-		if (hlen < sizeof(*iph) || htons(iph->tot_len) != l3_len ||
+		if (hlen < sizeof(*iph) || htons(iph->tot_len) > l3_len ||
 		    hlen > l3_len)
 			continue;
 
@@ -623,7 +623,7 @@ resume:
 		if (tap4_is_fragment(iph, now))
 			continue;
 
-		l4_len = l3_len - hlen;
+		l4_len = htons(iph->tot_len) - hlen;
 
 		if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr)
 			c->ip4.addr_seen.s_addr = iph->saddr;
-- 
@@ -615,7 +615,7 @@ resume:
 			continue;
 
 		hlen = iph->ihl * 4UL;
-		if (hlen < sizeof(*iph) || htons(iph->tot_len) != l3_len ||
+		if (hlen < sizeof(*iph) || htons(iph->tot_len) > l3_len ||
 		    hlen > l3_len)
 			continue;
 
@@ -623,7 +623,7 @@ resume:
 		if (tap4_is_fragment(iph, now))
 			continue;
 
-		l4_len = l3_len - hlen;
+		l4_len = htons(iph->tot_len) - hlen;
 
 		if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr)
 			c->ip4.addr_seen.s_addr = iph->saddr;
-- 
2.40.1

Re: [PATCH] tap: fix uses of l3_len in tap4_handler()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On Tue, Aug 29, 2023 at 09:44:06PM +0500, Stas Sergeev wrote:
> l3_len was calculated from the ethernet frame size, and it
> was assumed to be equal to the length stored in an IP packet.
> But if the ethernet frame is padded, then l3_len calculated
> that way can only be used as a bound check to validate the
> length stored in an IP header. It should not be used for
> calculating the l4_len.
> 
> This patch makes sure the small padded ethernet frames are
> properly processed, by trusting the length stored in an IP
> header.
> 
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> 
> CC: Stefano Brivio <sbrivio@redhat.com>
> ---
>  tap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index ee79be0..8d7859c 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -615,7 +615,7 @@ resume:
>  			continue;
>  
>  		hlen = iph->ihl * 4UL;
> -		if (hlen < sizeof(*iph) || htons(iph->tot_len) != l3_len ||
> +		if (hlen < sizeof(*iph) || htons(iph->tot_len) > l3_len ||
>  		    hlen > l3_len)
>  			continue;
>  
> @@ -623,7 +623,7 @@ resume:
>  		if (tap4_is_fragment(iph, now))
>  			continue;
>  
> -		l4_len = l3_len - hlen;
> +		l4_len = htons(iph->tot_len) - hlen;
>  
>  		if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr)
>  			c->ip4.addr_seen.s_addr = iph->saddr;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: fix uses of l3_len in tap4_handler()

[Stefano Brivio]

On Tue, 29 Aug 2023 21:44:06 +0500
Stas Sergeev <stsp2@yandex.ru> wrote:

> l3_len was calculated from the ethernet frame size, and it
> was assumed to be equal to the length stored in an IP packet.
> But if the ethernet frame is padded, then l3_len calculated
> that way can only be used as a bound check to validate the
> length stored in an IP header. It should not be used for
> calculating the l4_len.
> 
> This patch makes sure the small padded ethernet frames are
> properly processed, by trusting the length stored in an IP
> header.
> 
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

Applied, thanks!

-- 
Stefano