netdev - Re: [PATCH] eexpress: Read buffer overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20090726.184555.56450167.davem@davemloft.net>
Date:	Sun, 26 Jul 2009 18:45:55 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	roel.kluin@...il.com
Cc:	netdev@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [PATCH] eexpress: Read buffer overflow

From: Roel Kluin <roel.kluin@...il.com>
Date: Sat, 25 Jul 2009 21:50:19 +0200

> -	for (i = 0; i < (sizeof(start_code)); i+=32) {
> -		int j;
> -		outw(i, ioaddr + SM_PTR);
> -		for (j = 0; j < 16; j+=2)
> -			outw(start_code[(i+j)/2],
> -			     ioaddr+0x4000+j);
> -		for (j = 0; j < 16; j+=2)
> -			outw(start_code[(i+j+16)/2],
> -			     ioaddr+0x8000+j);
> +	for (i = 0; i < ARRAY_SIZE(start_code); i += 16) {
> +		int j, jmax;
> +		outw(i * 2, ioaddr + SM_PTR);
> +
> +		jmax = min_t(int, 16, ARRAY_SIZE(start_code) - i);
> +		for (j = 0; j < jmax; j++)
> +			outw(start_code[i + j],
> +			     ioaddr + j * 2 + (j < 8 ? 0x4000 : 0x8000 - 16));

This new IO address expression:

	ioaddr + j * 2 + (j < 8 ? 0x4000 : 0x8000 - 16)

DOES NOT match what the existing code does.  That 0x8000 - 16 seems
incorrect, the existing code always writes starting at 0x8000
in two byte increments, it does not start at 0x8000 - 16....

Oh nevermind, I see what you're doing.  Once j gets to 8, we have
to account for that in the IO address computation.

You've murdered this code, it's even more obfuscated now than it was
previously.  I'm not applying this, no way.  To a fix an out of
bounds array access you're going to change the loop iteration factors
and all of the sub-expressions within' 3 loops too?  Get real.

Just add the necessary limit tests, and nothing more, so it's
possible to actually understand your patch.  If it's more than
a 3 line patch, I'm not even going to review it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html