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] [day] [month] [year] [list]

Message-ID: <20190603193806.GK15954@unicorn.suse.cz>
Date:   Mon, 3 Jun 2019 21:38:06 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>, linville@...hat.com,
        f.fainelli@...il.com
Subject: Re: [PATCH net] ethtool: fix potential userspace buffer overflow

On Mon, Jun 03, 2019 at 01:42:19PM -0400, Vivien Didelot wrote:
> 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?

I guess we can handle returned regs.len in a follow-up patch. Othere
than that (and the part of commit message discussed above), the patch
looks good to me.

Michal