: //////////////////////////////////////////////
: // 6: /drivers/net/wan/sdla.c::sdla_xfer //
: //////////////////////////////////////////////
:
I am not a maintainer of sdla driver, but being Cc'd with this
mail, I'll try to look at it.
: - tainted signed scalar mem.len passed to kmalloc and memset (1206 and
: 1211, or 1220 and 1223). Possibly minor because of kmalloc's
: implicit size check
Yes. The value mem.len is passed to kmalloc and the code immediately
returns -ENOMEM when kmalloc fails.
: Protected by NET_ADMIN caps, but definately needs some bound checking.
:
Depends on whether the kmalloc's internal checks are
sufficient or not.
: Jan, I think SDLA_MAX_DATA is the correct bound to check for here, can
: you confirm please?
:
I cannot because I don't know or even have this hardware.
However: looking at the definitions in include/linux/sdla.h and the
code itself it looks like SDLA_READMEM, SDLA_WRITEMEM, and SDLA_CLEAR
ioctls are for reading/writing/clearing the card memory itself (maybe
for debugging purposes or downloading a firmware or what). So no,
SDLA_MAX_DATA is probably not a correct limit there.
The SDLA_CLEAR ioctl (the sdla_clear(dev) function) tries
to clear exactly 65536 bytes (hardcoded at sdla.c:sdla_clear() line 140).
So the mem.len should be <= 65536 bytes, and even mem.addr + mem.len
should be <= 65536 bytes.
So I propose the following patch (maybe the constant 65536 should
be defined in sdla.h and used both in sdla_xfer() and sdla_clear()):
Signed-off-by: Jan "Yenya" Kasprzak <[email protected]>
--- linux-2.4.28/drivers/net/wan/sdla.c.orig 2002-11-29 00:53:14.000000000 +0100
+++ linux-2.4.28/drivers/net/wan/sdla.c 2005-01-06 10:14:21.115490248 +0100
@@ -1195,6 +1195,10 @@
if(copy_from_user(&mem, info, sizeof(mem)))
return -EFAULT;
+
+ if (mem.len <= 0 || mem.addr < 0 || mem.len > 65536 || mem.addr > 65535
+ || mem.addr + mem.len > 65536)
+ return -EFAULT;
if (read)
{
--
| Jan "Yenya" Kasprzak <kas at {fi.muni.cz - work | yenya.net - private}> |
| GPG: ID 1024/D3498839 Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/ Czech Linux Homepage: http://www.linux.cz/ |
> Whatever the Java applications and desktop dances may lead to, Unix will <
> still be pushing the packets around for a quite a while. --Rob Pike <
Jan Kasprzak wrote:
> [...]
> + if (mem.len <= 0 || mem.addr < 0 || mem.len > 65536 || mem.addr > 65535
> + || mem.addr + mem.len > 65536)
> + return -EFAULT;
Just an extremely small nitpick. The conditions
mem.len > 65536 || mem.addr > 65535
aren't needed, because if one of them is true, then
mem.addr + mem.len > 65536
must be true also, since we've already asserted that len>0 and addr>=0.
This would be even simpler if len and addr were unsigned as they should
be, but that's probably not your fault :(
--
Paulo Marques - http://www.grupopie.com
"A journey of a thousand miles begins with a single step."
Lao-tzu, The Way of Lao-tzu
> The SDLA_CLEAR ioctl (the sdla_clear(dev) function) tries
> to clear exactly 65536 bytes (hardcoded at sdla.c:sdla_clear() line 140).
> So the mem.len should be <= 65536 bytes, and even mem.addr + mem.len
> should be <= 65536 bytes.
>
> So I propose the following patch (maybe the constant 65536 should
> be defined in sdla.h and used both in sdla_xfer() and sdla_clear()):
I'd propose they are set to CAP_SYS_RAWIO for those specific
debugging/firmware load operations. They allow access to that level
potentially anyway. That solves the problem cleanly.