| Message ID | 20101007211004.GA20267@outflux.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 An audit is needed, but first driver I checked is buggy (drivers/net/qlcnic/qlcnic_ethtool.c) -> memset(p, 0, qlcnic_get_regs_len(dev)); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-10-07 at 14:10 -0700, Kees Cook wrote: > 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. > > Cc: stable@kernel.org > Signed-off-by: Kees Cook <kees.cook@canonical.com> > --- > net/core/ethtool.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 7a85367..fb9cf30 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -397,7 +397,7 @@ static noinline_for_stack int ethtool_get_rxfh_indir(struct net_device *dev, > (KMALLOC_MAX_SIZE - sizeof(*indir)) / sizeof(*indir->ring_index)) > return -ENOMEM; > full_size = sizeof(*indir) + sizeof(*indir->ring_index) * table_size; > - indir = kmalloc(full_size, GFP_USER); > + indir = kzalloc(full_size, GFP_USER); > if (!indir) > return -ENOMEM; > [...] Acked-by: Ben Hutchings <bhutchings@solarflare.com> You could alternately recalculate full_size before copying back to the user buffer: full_size = sizeof(*indir) + sizeof(*indir->ring_index) * indir->size; but kzalloc() is more obviously safe. Ben.
On Thu, 2010-10-07 at 23:31 +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); Actually, I recently changed this to vmalloc() so your patch won't apply. > > 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 [...] Why should the driver's get_regs() check regs.len? The buffer is allocated based on reglen which is provided by the driver, not the user. reglen (length of the kernel buffer) is not reduced; regs.len (length of the user buffer) is. That lets the user know how much of the user buffer was actually used. Ben.
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
From: Ben Hutchings <bhutchings@solarflare.com> Date: Thu, 07 Oct 2010 22:34:44 +0100 > On Thu, 2010-10-07 at 14:10 -0700, Kees Cook wrote: >> 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. >> >> Cc: stable@kernel.org >> Signed-off-by: Kees Cook <kees.cook@canonical.com> ... > Acked-by: Ben Hutchings <bhutchings@solarflare.com> So I've applied Kees's patch to net-2.6, and I'll merge net-2.6 into net-next-2.6 so I can resolve the vmalloc/kzalloc merge conflict before Stephen Rothwell and others have to deal with it in -next. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 7a85367..fb9cf30 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -397,7 +397,7 @@ static noinline_for_stack int ethtool_get_rxfh_indir(struct net_device *dev, (KMALLOC_MAX_SIZE - sizeof(*indir)) / sizeof(*indir->ring_index)) return -ENOMEM; full_size = sizeof(*indir) + sizeof(*indir->ring_index) * table_size; - indir = kmalloc(full_size, GFP_USER); + indir = kzalloc(full_size, GFP_USER); if (!indir) return -ENOMEM; @@ -538,7 +538,7 @@ static int ethtool_get_rx_ntuple(struct net_device *dev, void __user *useraddr) gstrings.len = ret; - data = kmalloc(gstrings.len * ETH_GSTRING_LEN, GFP_USER); + data = kzalloc(gstrings.len * ETH_GSTRING_LEN, GFP_USER); if (!data) return -ENOMEM; @@ -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;
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. Cc: stable@kernel.org Signed-off-by: Kees Cook <kees.cook@canonical.com> --- net/core/ethtool.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)