diff mbox series

[1/2] airo: Fix possible info leak in AIROOLDIOCTL/SIOCDEVPRIVATE

Message ID 20200122040728.8437-1-mpe@ellerman.id.au (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show
Series [1/2] airo: Fix possible info leak in AIROOLDIOCTL/SIOCDEVPRIVATE | expand

Commit Message

Michael Ellerman Jan. 22, 2020, 4:07 a.m. UTC
The driver for Cisco Aironet 4500 and 4800 series cards (airo.c),
implements AIROOLDIOCTL/SIOCDEVPRIVATE in airo_ioctl().

The ioctl handler copies an aironet_ioctl struct from userspace, which
includes a command and a length. Some of the commands are handled in
readrids(), which kmalloc()'s a buffer of RIDSIZE (2048) bytes.

That buffer is then passed to PC4500_readrid(), which has two cases.
The else case does some setup and then reads up to RIDSIZE bytes from
the hardware into the kmalloc()'ed buffer.

Here len == RIDSIZE, pBuf is the kmalloc()'ed buffer:

	// read the rid length field
	bap_read(ai, pBuf, 2, BAP1);
	// length for remaining part of rid
	len = min(len, (int)le16_to_cpu(*(__le16*)pBuf)) - 2;
	...
	// read remainder of the rid
	rc = bap_read(ai, ((__le16*)pBuf)+1, len, BAP1);

PC4500_readrid() then returns to readrids() which does:

	len = comp->len;
	if (copy_to_user(comp->data, iobuf, min(len, (int)RIDSIZE))) {

Where comp->len is the user controlled length field.

So if the "rid length field" returned by the hardware is < 2048, and
the user requests 2048 bytes in comp->len, we will leak the previous
contents of the kmalloc()'ed buffer to userspace.

Fix it by kzalloc()'ing the buffer.

Found by Ilja by code inspection, not tested as I don't have the
required hardware.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 drivers/net/wireless/cisco/airo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

David Miller Jan. 23, 2020, 10:03 a.m. UTC | #1
From: Michael Ellerman <mpe@ellerman.id.au>
Date: Wed, 22 Jan 2020 15:07:27 +1100

> The driver for Cisco Aironet 4500 and 4800 series cards (airo.c),
> implements AIROOLDIOCTL/SIOCDEVPRIVATE in airo_ioctl().
> 
> The ioctl handler copies an aironet_ioctl struct from userspace, which
> includes a command and a length. Some of the commands are handled in
> readrids(), which kmalloc()'s a buffer of RIDSIZE (2048) bytes.
> 
> That buffer is then passed to PC4500_readrid(), which has two cases.
> The else case does some setup and then reads up to RIDSIZE bytes from
> the hardware into the kmalloc()'ed buffer.
> 
> Here len == RIDSIZE, pBuf is the kmalloc()'ed buffer:
> 
> 	// read the rid length field
> 	bap_read(ai, pBuf, 2, BAP1);
> 	// length for remaining part of rid
> 	len = min(len, (int)le16_to_cpu(*(__le16*)pBuf)) - 2;
> 	...
> 	// read remainder of the rid
> 	rc = bap_read(ai, ((__le16*)pBuf)+1, len, BAP1);
> 
> PC4500_readrid() then returns to readrids() which does:
> 
> 	len = comp->len;
> 	if (copy_to_user(comp->data, iobuf, min(len, (int)RIDSIZE))) {
> 
> Where comp->len is the user controlled length field.
> 
> So if the "rid length field" returned by the hardware is < 2048, and
> the user requests 2048 bytes in comp->len, we will leak the previous
> contents of the kmalloc()'ed buffer to userspace.
> 
> Fix it by kzalloc()'ing the buffer.
> 
> Found by Ilja by code inspection, not tested as I don't have the
> required hardware.
> 
> Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Applied and queued up for -stable.
diff mbox series

Patch

diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
index f43c06569ea1..d69c2ee7e206 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -7813,7 +7813,7 @@  static int readrids(struct net_device *dev, aironet_ioctl *comp) {
 		return -EINVAL;
 	}
 
-	if ((iobuf = kmalloc(RIDSIZE, GFP_KERNEL)) == NULL)
+	if ((iobuf = kzalloc(RIDSIZE, GFP_KERNEL)) == NULL)
 		return -ENOMEM;
 
 	PC4500_readrid(ai,ridcode,iobuf,RIDSIZE, 1);