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 <mkube...@suse.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.dide...@gmail.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(®s, 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, ®s, regbuf);
> > >
> > > ret = -EFAULT; if (copy_to_user(useraddr, ®s, 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
