Re: libsa's in_cksum() cannot handle payload of odd-length?
> > ./usr.sbin/eigrpd/in_cksum.c > > This one uses unsigned char instead of u_char, but is otherwise the same > as dvmrpd/ospfd. Should probably be made identical.
Re: libsa's in_cksum() cannot handle payload of odd-length?
On Mon, May 18, 2020 at 10:16:27AM -0600, Theo de Raadt wrote: > I suspect there are other inconsistancies in all these versions. > > ./sys/arch/arm/arm/in_cksum_arm.S > ./sys/arch/i386/i386/in_cksum.s > ./sys/arch/sparc64/sparc64/in_cksum.S > ./sys/arch/sh/sh/in_cksum.S These are assembly and for the kernel, uses mbufs. > ./sys/arch/alpha/alpha/in_cksum.c Has quite a bit of magic. > ./sys/arch/hppa/hppa/in_cksum.c Uses assembly in macros. > ./sys/arch/m88k/m88k/in_cksum.c Similar to hppa, but less obscure > ./sys/arch/powerpc/powerpc/in_cksum.c Plenty of powerpc assembly. > ./sys/netinet/in_cksum.c The default for the kernel. > ./usr.sbin/dvmrpd/in_cksum.c > ./usr.sbin/ospfd/in_cksum.c Finally some userland tools, so no mbufs. These are the same files, apart from dvmrpd.h/ospfd.h include. > ./usr.sbin/eigrpd/in_cksum.c This one uses unsigned char instead of u_char, but is otherwise the same as dvmrpd/ospfd. > ./usr.sbin/tcpdump/in_cksum.c Similar but has different requirements. > ./sys/lib/libsa/in_cksum.c Bootloader, no mbufs. Code best taken from one of the userland tools. ospfd is still the best bet.
Re: libsa's in_cksum() cannot handle payload of odd-length?
I suspect there are other inconsistancies in all these versions. ./usr.sbin/dvmrpd/in_cksum.c ./usr.sbin/eigrpd/in_cksum.c ./usr.sbin/ospfd/in_cksum.c ./usr.sbin/tcpdump/in_cksum.c ./sys/arch/alpha/alpha/in_cksum.c ./sys/arch/arm/arm/in_cksum_arm.S ./sys/arch/hppa/hppa/in_cksum.c ./sys/arch/i386/i386/in_cksum.s ./sys/arch/m88k/m88k/in_cksum.c ./sys/arch/powerpc/powerpc/in_cksum.c ./sys/arch/sh/sh/in_cksum.S ./sys/arch/sparc64/sparc64/in_cksum.S ./sys/lib/libsa/in_cksum.c ./sys/netinet/in_cksum.c
Re: libsa's in_cksum() cannot handle payload of odd-length?
On Mon, May 18, 2020 at 05:50:28PM +0200, Claudio Jeker wrote:
> On Mon, May 18, 2020 at 03:50:05PM +0200, Patrick Wildt wrote:
> > Hi,
> >
> > I was trying to tftpboot and had an issue with files of odd-length.
> > As it turns out, I think the in_cksum() that's called for UDP payload
> > cannot handle a payload length that's not aligned to 16 bytes.
> >
> > I don't know how in_cksum() is supposed to work exactly, but it looks
> > like the first step is summing up all bytes. The code is using 16-
> > byte blocks, apart from some oddbyte magic.
> >
> > First of all, why is there a while loop around code that already
> > consumes the whole length? That can be done in a single step
> > without the loop. Why does it continue of there's an "oddbyte"?
> >
> > If I simplify that whole construct, consuming in 16-bytes step
> > until there's only one left, then summing that one, in_cksum()
> > works for me.
> >
> > Can someone please help me have a look?
>
> There are other versions of in_cksum in our tree.
> Like: ./usr.sbin/ospfd/in_cksum.c
>
> I'm surprised that the libsa code does no htons / ntohs conversions.
> Also after looking at the ospfd/in_cksum.c code I wonder if the htons /
> ntohs are actually reversed in that that code...
I copied ospfd's file, re-added the header includes, re-added const to
the pointer, adjusted the prototype for u_int16_t and size_t, and
replaced the fatal with a printf and return -1. Why -1? Well, the
checks always do if (in_cksum() != 0) { error(); }. Unless they
create a packet, but I guess in that case I hope we don't create a
packet that's too big *in our bootloader*.
Since the implementation was replaced, taken from ospfd, I guess the
4th-clause can go as well? I mean, there's no 4th-clause in ospfd and
I just copied the file.
This fixes my issue and works for me.
Opinions? ok?
Patrick
diff --git a/sys/lib/libsa/in_cksum.c b/sys/lib/libsa/in_cksum.c
index d3f2e6ac978..c4b12e01c04 100644
--- a/sys/lib/libsa/in_cksum.c
+++ b/sys/lib/libsa/in_cksum.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: in_cksum.c,v 1.5 2014/11/19 20:28:56 miod Exp $ */
+/* $OpenBSD: in_cksum.c,v 1.7 2014/07/20 20:27:19 tobias Exp $ */
/* $NetBSD: in_cksum.c,v 1.3 1995/04/22 13:53:48 cgd Exp $ */
/*
@@ -17,11 +17,7 @@
* 2. Redistributions in binary form must reproduce the above copyright
*notice, this list of conditions and the following disclaimer in the
*documentation and/or other materials provided with the distribution.
- * 3. All advertising materials mentioning features or use of this software
- *must display the following acknowledgement:
- * This product includes software developed by the University of
- * California, Lawrence Berkeley Laboratory and its contributors.
- * 4. Neither the name of the University nor the names of its contributors
+ * 3. Neither the name of the University nor the names of its contributors
*may be used to endorse or promote products derived from this software
*without specific prior written permission.
*
@@ -56,35 +52,38 @@
* code and should be modified for each CPU to be as fast as possible.
* In particular, it should not be this one.
*/
-int
-in_cksum(const void *p, int len)
+u_int16_t
+in_cksum(const void *p, size_t l)
{
- int sum = 0, oddbyte = 0, v = 0;
+ unsigned int sum = 0;
+ int len;
const u_char *cp = p;
- /* we assume < 2^16 bytes being summed */
- while (len > 0) {
- if (oddbyte) {
- sum += v + *cp++;
- len--;
+ /* ensure that < 2^16 bytes being summed */
+ if (l >= (1 << 16)) {
+ printf("in_cksum: packet too big\n");
+ return -1;
+ }
+ len = (int)l;
+
+ if (((long)cp & 1) == 0) {
+ while (len > 1) {
+ sum += htons(*(u_short *)cp);
+ cp += 2;
+ len -= 2;
}
- if (((long)cp & 1) == 0) {
- while ((len -= 2) >= 0) {
- sum += *(const u_short *)cp;
- cp += 2;
- }
- } else {
- while ((len -= 2) >= 0) {
- sum += *cp++ << 8;
- sum += *cp++;
- }
+ } else {
+ while (len > 1) {
+ sum += *cp++ << 8;
+ sum += *cp++;
+ len -= 2;
}
- if ((oddbyte = len & 1) != 0)
- v = *cp << 8;
}
- if (oddbyte)
- sum += v;
+ if (len == 1)
+ sum += *cp << 8;
+
sum = (sum >> 16) + (sum & 0x); /* add in accumulated carries */
sum += sum >> 16; /* add potential last carry */
+ sum = ntohs(sum);
return
Re: libsa's in_cksum() cannot handle payload of odd-length?
On Mon, May 18, 2020 at 03:50:05PM +0200, Patrick Wildt wrote:
> Hi,
>
> I was trying to tftpboot and had an issue with files of odd-length.
> As it turns out, I think the in_cksum() that's called for UDP payload
> cannot handle a payload length that's not aligned to 16 bytes.
>
> I don't know how in_cksum() is supposed to work exactly, but it looks
> like the first step is summing up all bytes. The code is using 16-
> byte blocks, apart from some oddbyte magic.
>
> First of all, why is there a while loop around code that already
> consumes the whole length? That can be done in a single step
> without the loop. Why does it continue of there's an "oddbyte"?
>
> If I simplify that whole construct, consuming in 16-bytes step
> until there's only one left, then summing that one, in_cksum()
> works for me.
>
> Can someone please help me have a look?
There are other versions of in_cksum in our tree.
Like: ./usr.sbin/ospfd/in_cksum.c
I'm surprised that the libsa code does no htons / ntohs conversions.
Also after looking at the ospfd/in_cksum.c code I wonder if the htons /
ntohs are actually reversed in that that code...
> Patrick
>
> diff --git a/sys/lib/libsa/in_cksum.c b/sys/lib/libsa/in_cksum.c
> index d3f2e6ac978..57ded38a7b7 100644
> --- a/sys/lib/libsa/in_cksum.c
> +++ b/sys/lib/libsa/in_cksum.c
> @@ -59,31 +59,24 @@
> int
> in_cksum(const void *p, int len)
> {
> - int sum = 0, oddbyte = 0, v = 0;
> const u_char *cp = p;
> + int sum = 0;
>
> /* we assume < 2^16 bytes being summed */
> - while (len > 0) {
> - if (oddbyte) {
> - sum += v + *cp++;
> - len--;
> - }
> + while (len > 1) {
> if (((long)cp & 1) == 0) {
> - while ((len -= 2) >= 0) {
> - sum += *(const u_short *)cp;
> - cp += 2;
> - }
> + sum += *(const u_short *)cp;
> + cp += 2;
> } else {
> - while ((len -= 2) >= 0) {
> - sum += *cp++ << 8;
> - sum += *cp++;
> - }
> + sum += *cp++ << 8;
> + sum += *cp++;
> }
> - if ((oddbyte = len & 1) != 0)
> - v = *cp << 8;
> + len -= 2;
> + }
> + if (len > 0) {
> + sum += *cp++;
> + len--;
> }
> - if (oddbyte)
> - sum += v;
> sum = (sum >> 16) + (sum & 0x); /* add in accumulated carries */
> sum += sum >> 16; /* add potential last carry */
> return (0x & ~sum);
>
--
:wq Claudio
libsa's in_cksum() cannot handle payload of odd-length?
Hi,
I was trying to tftpboot and had an issue with files of odd-length.
As it turns out, I think the in_cksum() that's called for UDP payload
cannot handle a payload length that's not aligned to 16 bytes.
I don't know how in_cksum() is supposed to work exactly, but it looks
like the first step is summing up all bytes. The code is using 16-
byte blocks, apart from some oddbyte magic.
First of all, why is there a while loop around code that already
consumes the whole length? That can be done in a single step
without the loop. Why does it continue of there's an "oddbyte"?
If I simplify that whole construct, consuming in 16-bytes step
until there's only one left, then summing that one, in_cksum()
works for me.
Can someone please help me have a look?
Patrick
diff --git a/sys/lib/libsa/in_cksum.c b/sys/lib/libsa/in_cksum.c
index d3f2e6ac978..57ded38a7b7 100644
--- a/sys/lib/libsa/in_cksum.c
+++ b/sys/lib/libsa/in_cksum.c
@@ -59,31 +59,24 @@
int
in_cksum(const void *p, int len)
{
- int sum = 0, oddbyte = 0, v = 0;
const u_char *cp = p;
+ int sum = 0;
/* we assume < 2^16 bytes being summed */
- while (len > 0) {
- if (oddbyte) {
- sum += v + *cp++;
- len--;
- }
+ while (len > 1) {
if (((long)cp & 1) == 0) {
- while ((len -= 2) >= 0) {
- sum += *(const u_short *)cp;
- cp += 2;
- }
+ sum += *(const u_short *)cp;
+ cp += 2;
} else {
- while ((len -= 2) >= 0) {
- sum += *cp++ << 8;
- sum += *cp++;
- }
+ sum += *cp++ << 8;
+ sum += *cp++;
}
- if ((oddbyte = len & 1) != 0)
- v = *cp << 8;
+ len -= 2;
+ }
+ if (len > 0) {
+ sum += *cp++;
+ len--;
}
- if (oddbyte)
- sum += v;
sum = (sum >> 16) + (sum & 0x); /* add in accumulated carries */
sum += sum >> 16; /* add potential last carry */
return (0x & ~sum);
