netdev - Re: [PATCH net] ethtool: fix potential userspace buffer overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Jun 2019 13:42:19 -0400
From:   Vivien Didelot <vivien.didelot@...il.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        linville@...hat.com, f.fainelli@...il.com
Subject: Re: [PATCH net] ethtool: fix potential userspace buffer overflow

Hi Michal,

On Mon, 3 Jun 2019 19:15:12 +0200, Michal Kubecek <mkubecek@...e.cz> wrote:
> On Fri, May 31, 2019 at 07:12:21PM -0400, Vivien Didelot wrote:
> > ethtool_get_regs() allocates a buffer of size ops->get_regs_len(),
> > and pass it to the kernel driver via ops->get_regs() for filling.
> > 
> > There is no restriction about what the kernel drivers can or cannot do
> > with the open ethtool_regs structure. They usually set regs->version
> > and ignore regs->len or set it to the same size as ops->get_regs_len().
> > 
> > Userspace may actually allocate a smaller buffer for registers dump,
> > for instance ethtool does that when dumping the raw registers directly
> > into a fixed-size file.
> 
> This is not exactly true. AFAICS ethtool always calls the ETHTOOL_GREGS
> ioctl with the size returned by ETHTOOL_GDRVINFO. Only after that it may
> replace regs->len with file length but it's a file it _reads_ and
> replaces the dump (or part of it) with it. (Which doesn't seem to make
> sense: if ethtool(8) man page says ethtool is to display registers from
> a previously saved dump, why does ethtool execute the ETHTOOL_GREGS
> request at all?)

You are correct, what ethtool does here is weird. I can remove this statement
from the commit message in a v2.

>  
> > Because the current code uses the regs.len value potentially updated
> > by the driver when copying the buffer back to userspace, we may
> > actually cause a userspace buffer overflow in the final copy_to_user()
> > call.
> > 
> > To fix this, make this case obvious and store regs.len before calling
> > ops->get_regs(), to only copy as much data as requested by userspace,
> > up to the value returned by ops->get_regs_len().
> > 
> > While at it, remove the redundant check for non-null regbuf.
> > 
> > Signed-off-by: Vivien Didelot <vivien.didelot@...il.com> ---
> > net/core/ethtool.c | 5 ++++- 1 file changed, 4 insertions(+), 1
> > deletion(-)
> > 
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
> > 43e9add58340..1a0196fbb49c 100644 --- a/net/core/ethtool.c +++
> > b/net/core/ethtool.c @@ -1338,38 +1338,41 @@ static noinline_for_stack
> > int ethtool_set_rxfh(struct net_device *dev, static int
> > ethtool_get_regs(struct net_device *dev, char __user *useraddr) {
> > struct ethtool_regs regs; const struct ethtool_ops *ops =
> > dev->ethtool_ops; void *regbuf; int reglen, ret;
> >  
> >  	if (!ops->get_regs || !ops->get_regs_len) return -EOPNOTSUPP;
> >  
> >  	if (copy_from_user(&regs, useraddr, sizeof(regs))) return
> >  	-EFAULT;
> >  
> >  	reglen = ops->get_regs_len(dev); if (reglen <= 0) return reglen;
> >  
> >  	if (regs.len > reglen) regs.len = reglen;
> >  
> >  	regbuf = vzalloc(reglen); if (!regbuf) return -ENOMEM;
> >  
> > +	if (regs.len < reglen) +		reglen = regs.len; +
> > ops->get_regs(dev, &regs, regbuf);
> >  
> >  	ret = -EFAULT; if (copy_to_user(useraddr, &regs, sizeof(regs)))
> >  	goto out; useraddr += offsetof(struct ethtool_regs, data); -
> >  	if (regbuf && copy_to_user(useraddr, regbuf, regs.len)) +
> >  	if (copy_to_user(useraddr, regbuf, reglen)) goto out; ret = 0;
> >  
> >   out: vfree(regbuf); return ret; }
> 
> Yes, this will address overflowing the userspace buffer. It will still
> either preserve regs.len or replace it with full dump length, depending
> on the driver. But to address that, we should first agree which it
> should be. I'm afraid there is no good choice, setting regs.len to size
> actually returned is IMHO less bad.

I don't think it is necessary to change the current behavior of the drivers
for the moment. regs.len is kept the same, so we don't impact userspace as
well. But what's important is to not use regs.len after ops->get_regs()
is called, because we must use the value passed by userspace (up to
ops->get_regs_len() for sure). That is what this patch focuses on.

Do you want me to change something beside rewording the commit message?


Thank you,
Vivien