[PATCH net-next] ethtool: do not use regs->len after ops->get_regs

[PATCH net-next] ethtool: do not use regs->len after ops->get_regs

[Vivien Didelot]

The kernel 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 regs->len member. Drivers usually ignore it or set
the same size again. However, ethtool_get_regs() must not use this
value when copying the buffer back to the user, because userspace may
have allocated a smaller buffer. For instance ethtool does that when
dumping the raw registers directly into a fixed-size file.

Software may still make use of the regs->len value updated by the
kernel driver, but ethtool_get_regs() must use the original regs->len
given by userspace, up to ops->get_regs_len(), when copying the buffer.

Also no need to check regbuf twice.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 net/core/ethtool.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 4a593853cbf2..8f95c7b7cafe 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1338,38 +1338,40 @@ 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;
+	else
+		reglen = regs.len;
 
 	regbuf = vzalloc(reglen);
 	if (!regbuf)
 		return -ENOMEM;
 
 	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;
 }
-- 
2.21.0

Re: [PATCH net-next] ethtool: do not use regs->len after ops->get_regs

[Michal Kubecek]

On Thu, May 30, 2019 at 07:54:50PM -0400, Vivien Didelot wrote:
> The kernel 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 regs->len member. Drivers usually ignore it or set
> the same size again. However, ethtool_get_regs() must not use this
> value when copying the buffer back to the user, because userspace may
> have allocated a smaller buffer. For instance ethtool does that when
> dumping the raw registers directly into a fixed-size file.
> 
> Software may still make use of the regs->len value updated by the
> kernel driver, but ethtool_get_regs() must use the original regs->len
> given by userspace, up to ops->get_regs_len(), when copying the buffer.
> 
> Also no need to check regbuf twice.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---
>  net/core/ethtool.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 4a593853cbf2..8f95c7b7cafe 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1338,38 +1338,40 @@ 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;
> +	else
> +		reglen = regs.len;

This seems wrong. Most drivers do not check regs.len in their get_regs()
handler (I'm not sure if there are any that do) and simply write as much
data as they have. Thus if userspace passes too short regs.len, this
would replace overflow of a userspace buffer for few drivers by overflow
of a kernel buffer for (almost) all drivers.

So while we should use the original regs.len from userspace for final
copy_to_user(), we have to allocate the buffer for driver ->get_regs()
callback with size returned by its ->get_regs_len() callback.

Michal Kubecek

>  
>  	regbuf = vzalloc(reglen);
>  	if (!regbuf)
>  		return -ENOMEM;
>  
>  	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;
>  }
> -- 
> 2.21.0
> 

Re: [PATCH net-next] ethtool: do not use regs->len after ops->get_regs

[Vivien Didelot]

Hi Michal,

On Fri, 31 May 2019 08:54:32 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Thu, May 30, 2019 at 07:54:50PM -0400, Vivien Didelot wrote:
> > The kernel 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 regs->len member. Drivers usually ignore it or set
> > the same size again. However, ethtool_get_regs() must not use this
> > value when copying the buffer back to the user, because userspace may
> > have allocated a smaller buffer. For instance ethtool does that when
> > dumping the raw registers directly into a fixed-size file.
> > 
> > Software may still make use of the regs->len value updated by the
> > kernel driver, but ethtool_get_regs() must use the original regs->len
> > given by userspace, up to ops->get_regs_len(), when copying the buffer.
> > 
> > Also no need to check regbuf twice.
> > 
> > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > ---
> >  net/core/ethtool.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 4a593853cbf2..8f95c7b7cafe 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1338,38 +1338,40 @@ 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;
> > +	else
> > +		reglen = regs.len;
> 
> This seems wrong. Most drivers do not check regs.len in their get_regs()
> handler (I'm not sure if there are any that do) and simply write as much
> data as they have. Thus if userspace passes too short regs.len, this
> would replace overflow of a userspace buffer for few drivers by overflow
> of a kernel buffer for (almost) all drivers.
> 
> So while we should use the original regs.len from userspace for final
> copy_to_user(), we have to allocate the buffer for driver ->get_regs()
> callback with size returned by its ->get_regs_len() callback.

Either I've completely screwed my patch, or you have misread it. This patch
actually just stores the original value of regs.len passed by userspace to
the kernel into reglen, before calling ops->get_regs().

But the kernel still allocates ops->get_regs_len() and passes that to the
kernel drivers, as this is the only size drivers must care about.

Then the kernel only copies what the userspace (originally) requested,
up to ops->get_regs_len().

In other words, if userspace requested a bigger buffer only ops->get_regs_len()
get copied, if the userspace requested a smaller buffer only that length
get copied.


Thanks,
Vivien

Re: [PATCH net-next] ethtool: do not use regs->len after ops->get_regs

[Michal Kubecek]

On Fri, May 31, 2019 at 10:15:01AM -0400, Vivien Didelot wrote:
> Hi Michal,
> 
> On Fri, 31 May 2019 08:54:32 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> > On Thu, May 30, 2019 at 07:54:50PM -0400, Vivien Didelot wrote:
> > > The kernel 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 regs->len member. Drivers usually ignore it or set
> > > the same size again. However, ethtool_get_regs() must not use this
> > > value when copying the buffer back to the user, because userspace may
> > > have allocated a smaller buffer. For instance ethtool does that when
> > > dumping the raw registers directly into a fixed-size file.
> > > 
> > > Software may still make use of the regs->len value updated by the
> > > kernel driver, but ethtool_get_regs() must use the original regs->len
> > > given by userspace, up to ops->get_regs_len(), when copying the buffer.
> > > 
> > > Also no need to check regbuf twice.
> > > 
> > > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > > ---
> > >  net/core/ethtool.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > > index 4a593853cbf2..8f95c7b7cafe 100644
> > > --- a/net/core/ethtool.c
> > > +++ b/net/core/ethtool.c
> > > @@ -1338,38 +1338,40 @@ 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;
> > > +	else
> > > +		reglen = regs.len;
> > 
> > This seems wrong. Most drivers do not check regs.len in their get_regs()
> > handler (I'm not sure if there are any that do) and simply write as much
> > data as they have. Thus if userspace passes too short regs.len, this
> > would replace overflow of a userspace buffer for few drivers by overflow
> > of a kernel buffer for (almost) all drivers.
> > 
> > So while we should use the original regs.len from userspace for final
> > copy_to_user(), we have to allocate the buffer for driver ->get_regs()
> > callback with size returned by its ->get_regs_len() callback.
> 
> Either I've completely screwed my patch, or you have misread it. This patch
> actually just stores the original value of regs.len passed by userspace to
> the kernel into reglen, before calling ops->get_regs().
> 
> But the kernel still allocates ops->get_regs_len() and passes that to the
> kernel drivers, as this is the only size drivers must care about.
> 
> Then the kernel only copies what the userspace (originally) requested,
> up to ops->get_regs_len().
> 
> In other words, if userspace requested a bigger buffer only ops->get_regs_len()
> get copied, if the userspace requested a smaller buffer only that length
> get copied.

The problem with this patch is not with what gets copied to userspace
but with the buffer allocated for driver callback. With this patch, the
code looks like this:

	reglen = ops->get_regs_len(dev);
	if (reglen <= 0)
		return reglen;

Here we get actual dump size from driver and put it into reglen.

	if (regs.len > reglen)
		regs.len = reglen;
	else
		reglen = regs.len;

If userspace buffer is insufficient, i.e. regs.len < reglen, we shrink
reglen to regs.len.

	regbuf = vzalloc(reglen);
	if (!regbuf)
		return -ENOMEM;

Here we allocate a buffer of size reglen (which has been shrunk to
regs.len from userspace).

	ops->get_regs(dev, &regs, regbuf);

And pass that buffer to driver's ->get_regs() callback. But these
callbacks mostly ignore regs.len and simply write as much data as they
have (size equal to what ->get_regs_len() returned). So if regs.len
provided by userspace is strictly shorter than actual dump size, driver
writes past the buffer allocated above.

Michal

Re: [PATCH net-next] ethtool: do not use regs->len after ops->get_regs

[Vivien Didelot]

Hi Michal,

On Fri, 31 May 2019 16:27:50 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Fri, May 31, 2019 at 10:15:01AM -0400, Vivien Didelot wrote:
> > Hi Michal,
> > 
> > On Fri, 31 May 2019 08:54:32 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> > > On Thu, May 30, 2019 at 07:54:50PM -0400, Vivien Didelot wrote:
> > > > The kernel 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 regs->len member. Drivers usually ignore it or set
> > > > the same size again. However, ethtool_get_regs() must not use this
> > > > value when copying the buffer back to the user, because userspace may
> > > > have allocated a smaller buffer. For instance ethtool does that when
> > > > dumping the raw registers directly into a fixed-size file.
> > > > 
> > > > Software may still make use of the regs->len value updated by the
> > > > kernel driver, but ethtool_get_regs() must use the original regs->len
> > > > given by userspace, up to ops->get_regs_len(), when copying the buffer.
> > > > 
> > > > Also no need to check regbuf twice.
> > > > 
> > > > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > > > ---
> > > >  net/core/ethtool.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > > > index 4a593853cbf2..8f95c7b7cafe 100644
> > > > --- a/net/core/ethtool.c
> > > > +++ b/net/core/ethtool.c
> > > > @@ -1338,38 +1338,40 @@ 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;
> > > > +	else
> > > > +		reglen = regs.len;
> > > 
> > > This seems wrong. Most drivers do not check regs.len in their get_regs()
> > > handler (I'm not sure if there are any that do) and simply write as much
> > > data as they have. Thus if userspace passes too short regs.len, this
> > > would replace overflow of a userspace buffer for few drivers by overflow
> > > of a kernel buffer for (almost) all drivers.
> > > 
> > > So while we should use the original regs.len from userspace for final
> > > copy_to_user(), we have to allocate the buffer for driver ->get_regs()
> > > callback with size returned by its ->get_regs_len() callback.
> > 
> > Either I've completely screwed my patch, or you have misread it. This patch
> > actually just stores the original value of regs.len passed by userspace to
> > the kernel into reglen, before calling ops->get_regs().
> > 
> > But the kernel still allocates ops->get_regs_len() and passes that to the
> > kernel drivers, as this is the only size drivers must care about.
> > 
> > Then the kernel only copies what the userspace (originally) requested,
> > up to ops->get_regs_len().
> > 
> > In other words, if userspace requested a bigger buffer only ops->get_regs_len()
> > get copied, if the userspace requested a smaller buffer only that length
> > get copied.
> 
> The problem with this patch is not with what gets copied to userspace
> but with the buffer allocated for driver callback. With this patch, the
> code looks like this:
> 
> 	reglen = ops->get_regs_len(dev);
> 	if (reglen <= 0)
> 		return reglen;
> 
> Here we get actual dump size from driver and put it into reglen.
> 
> 	if (regs.len > reglen)
> 		regs.len = reglen;
> 	else
> 		reglen = regs.len;
> 
> If userspace buffer is insufficient, i.e. regs.len < reglen, we shrink
> reglen to regs.len.
> 
> 	regbuf = vzalloc(reglen);
> 	if (!regbuf)
> 		return -ENOMEM;
> 
> Here we allocate a buffer of size reglen (which has been shrunk to
> regs.len from userspace).
> 
> 	ops->get_regs(dev, &regs, regbuf);
> 
> And pass that buffer to driver's ->get_regs() callback. But these
> callbacks mostly ignore regs.len and simply write as much data as they
> have (size equal to what ->get_regs_len() returned). So if regs.len
> provided by userspace is strictly shorter than actual dump size, driver
> writes past the buffer allocated above.

I've screwed my patch, the reglen assignment must go after the allocation
for sure... sorry about that. What my fix was supposed to be was:

    diff --git a/net/core/ethtool.c b/net/core/ethtool.c
    index 4a593853cbf2..36057ec807f4 100644
    --- a/net/core/ethtool.c
    +++ b/net/core/ethtool.c
    @@ -1359,13 +1359,16 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
            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;
 
So that we simply store the original regs.len (up to ops->get_regs_len())
before calling ops->get_regs().


Thanks,
Vivien