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]

Message-ID: <20190603171512.GJ15954@unicorn.suse.cz>
Date:   Mon, 3 Jun 2019 19:15:12 +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 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?)
 
> 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.

Michal