Messages in this thread |  | | Date | Thu, 7 Oct 2010 14:40:21 -0700 | From | Kees Cook <> | Subject | Re: [PATCH] net: clear heap allocations for privileged ethtool actions |
| |
Hi Eric,
On Thu, Oct 07, 2010 at 11:31:25PM +0200, Eric Dumazet wrote: > Le jeudi 07 octobre 2010 à 14:10 -0700, Kees Cook a écrit : > > Several other ethtool functions leave heap uncleared (potentially) by > > drivers. Some interfaces appear safe (eeprom, etc), in that the sizes > > are well controlled. In some situations (e.g. unchecked error conditions), > > the heap will remain unchanged in areas before copying back to userspace. > > Note that these are less of an issue since these all require CAP_NET_ADMIN. > > > @@ -775,7 +775,7 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr) > > if (regs.len > reglen) > > regs.len = reglen; > > > > - regbuf = kmalloc(reglen, GFP_USER); > > + regbuf = kzalloc(reglen, GFP_USER); > > if (!regbuf) > > return -ENOMEM; > > > > -- > > 1.7.1 > > > > Are you sure this is not hiding a more problematic problem ? > > Code does : > > reglen = ops->get_regs_len(dev); > if (regs.len > reglen) > regs.len = reglen; > regbuf = kmalloc(reglen, GFP_USER); > > So we can not copy back kernel memory. > > However, what happens if user provides regs.len = 1 byte, and driver > get_regs() doesnt properly checks regs.len and write past end of regbuf > -> We probably write on other parts of kernel memory
This code is basically a max() call from what I see.
regbuf = kmalloc(max(regs.len, ops->get_regs_len(dev)), GFP_USER);
If the user passes regs.len = 1, it will be ignored in favor of reglen, so we'll not write past the end of regbuf, unless I'm misunderstanding.
-Kees
-- Kees Cook Ubuntu Security Team -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |