[U-Boot] Bug in IP/UDP defragment?

[Alessandro Rubini]

Hello.
Please forgive my delay.

> 16:58:32.290407 IP 10.12.48.10.33088 > 10.12.48.32.3285: UDP, length 2959
> 16:58:32.290410 IP 10.12.48.10 > 10.12.48.32: udp
> 16:58:32.290412 IP 10.12.48.10 > 10.12.48.32: udp

The third fragment here is less than 8 bytes of payload, and this
triggers the bug.

> I don't know yet what are the side effects of my patch.

Ok, I restudied the flow.

> -	if (offset8 + (len / 8) <= h - payload) {
> +	if (offset8 + (len / 8) < h - payload) {
>  		/* no overlap with holes (dup fragment?) */
>  		return NULL;

offset8 is the start of this fragment, as 8-byte-items, and h is the
current hole, the one that might be relevant. Since everything before
"h" is already filled, a fragment that ends where this hole starts
is duplicate, thus "<=". 

Here len/8 is 0, so you force accepting this trailing fragment, but
you'll also accept duplicate packets.  Looking at how following code
behaves in this case (it assumes some hole or part of is filled),
the "overlaps with initial part of the hole" will trigger, and the
hole will be rewritten unchanged.

So I don't see bad side effects in your fix, but if anyone will re-read
the code (looking for another bug, for example) after your change it will
be hard to understand (they'll suggest reverting the change for example).

I'd suggest to still discard duplicate fragments, by turning

	if (offset8 + (len / 8) <= h - payload)

to
	/* last fragment may be 1..7 bytes, the "+7" forces acceptance */
	if (offset8 + ((len + 7) / 8) <= h - payload)

or something along these lines. Will you please post a patch?

Thanks a lof for your report and fix

/alessandro