[PATCH] net: fix checksum verification

[PATCH] net: fix checksum verification

[Baruch Siach]

Checksum verification on data including its own checksum (as is the case with
IP headers) should give zero. Current code works well for the correct checksum
case, but fails to identify (most) errors.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---

Untested. From code inspection only.

 net/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index 0bd9084..bd7a578 100644
--- a/net/net.c
+++ b/net/net.c
@@ -41,7 +41,7 @@ static unsigned int net_ip_id;
 
 int net_checksum_ok(unsigned char *ptr, int len)
 {
-	return net_checksum(ptr, len) + 1;
+	return net_checksum(ptr, len) == 0;
 }
 
 uint16_t net_checksum(unsigned char *ptr, int len)
-- 
1.8.4.rc1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: fix checksum verification

[Sascha Hauer]

On Fri, Aug 09, 2013 at 07:01:24AM +0300, Baruch Siach wrote:
> Checksum verification on data including its own checksum (as is the case with
> IP headers) should give zero. Current code works well for the correct checksum
> case, but fails to identify (most) errors.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> 
> Untested. From code inspection only.
> 
>  net/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/net.c b/net/net.c
> index 0bd9084..bd7a578 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -41,7 +41,7 @@ static unsigned int net_ip_id;
>  
>  int net_checksum_ok(unsigned char *ptr, int len)
>  {
> -	return net_checksum(ptr, len) + 1;
> +	return net_checksum(ptr, len) == 0;

D'oh. There's a bug indeed. For a good packet net_checksum returns
0xffff (all ones in an u16). So the check should be:

	return net_checksum(ptr, len) == 0xffff;

U-Boot has this instead:

	return !((net_checksum(ptr, len) + 1) & 0xfffe);

From what I see both above should be equivalent so I wonder why U-Boot
has such a complicated code here. Some compiler optimization or is this
something I don't see?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: fix checksum verification

[Uwe Kleine-König]

On Fri, Aug 09, 2013 at 09:32:37AM +0200, Sascha Hauer wrote:
> On Fri, Aug 09, 2013 at 07:01:24AM +0300, Baruch Siach wrote:
> > Checksum verification on data including its own checksum (as is the case with
> > IP headers) should give zero. Current code works well for the correct checksum
> > case, but fails to identify (most) errors.
> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> > 
> > Untested. From code inspection only.
> > 
> >  net/net.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index 0bd9084..bd7a578 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -41,7 +41,7 @@ static unsigned int net_ip_id;
> >  
> >  int net_checksum_ok(unsigned char *ptr, int len)
> >  {
> > -	return net_checksum(ptr, len) + 1;
> > +	return net_checksum(ptr, len) == 0;
> 
> D'oh. There's a bug indeed. For a good packet net_checksum returns
> 0xffff (all ones in an u16). So the check should be:
> 
> 	return net_checksum(ptr, len) == 0xffff;
with

	return net_checksum(ptr, len) + 1

net_checksum_ok returns always something >0 (i.e. success) because both
summands are converted to unsigned, and so never catches an error[1],
does it?

> U-Boot has this instead:
> 
> 	return !((net_checksum(ptr, len) + 1) & 0xfffe);
> 
> From what I see both above should be equivalent so I wonder why U-Boot
> has such a complicated code here. Some compiler optimization or is this
> something I don't see?
This isn't equivalent. The U-Boot code returns 1 iff net_checksum
returns 0 or 0xffff; 0 otherwise.

Best regards
Uwe

[1] well unless unsigned is only 16 bits wide which shouldn't be the case on
all platforms barebox is running on.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: fix checksum verification

[Sascha Hauer]

On Fri, Aug 09, 2013 at 10:50:19AM +0200, Uwe Kleine-König wrote:
> > D'oh. There's a bug indeed. For a good packet net_checksum returns
> > 0xffff (all ones in an u16). So the check should be:
> > 
> > 	return net_checksum(ptr, len) == 0xffff;
> with
> 
> 	return net_checksum(ptr, len) + 1
> 
> net_checksum_ok returns always something >0 (i.e. success) because both
> summands are converted to unsigned, and so never catches an error[1],
> does it?
> 
> > U-Boot has this instead:
> > 
> > 	return !((net_checksum(ptr, len) + 1) & 0xfffe);
> > 
> > From what I see both above should be equivalent so I wonder why U-Boot
> > has such a complicated code here. Some compiler optimization or is this
> > something I don't see?
> This isn't equivalent. The U-Boot code returns 1 iff net_checksum
> returns 0 or 0xffff; 0 otherwise.

Hm, indeed. Which brings me to my next question: In which cases does
net_checksum() return 0 and the result can be considered ok?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net: fix checksum verification

[Baruch Siach]

Hi Sascha,

On Fri, Aug 09, 2013 at 09:32:37AM +0200, Sascha Hauer wrote:
> On Fri, Aug 09, 2013 at 07:01:24AM +0300, Baruch Siach wrote:
> > Checksum verification on data including its own checksum (as is the case with
> > IP headers) should give zero. Current code works well for the correct checksum
> > case, but fails to identify (most) errors.
> > 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> > 
> > Untested. From code inspection only.
> > 
> >  net/net.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index 0bd9084..bd7a578 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -41,7 +41,7 @@ static unsigned int net_ip_id;
> >  
> >  int net_checksum_ok(unsigned char *ptr, int len)
> >  {
> > -	return net_checksum(ptr, len) + 1;
> > +	return net_checksum(ptr, len) == 0;
> 
> D'oh. There's a bug indeed. For a good packet net_checksum returns
> 0xffff (all ones in an u16). So the check should be:
> 
> 	return net_checksum(ptr, len) == 0xffff;

You're right, of course. I was just blindly copying the kernel that has this 
in ip_rcv():

    if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
        goto csum_error;

But I still don't understand the logic here.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox