| 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 |
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 --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);
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(-)