Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

[Kalle Valo]

David Miller  writes:

> From: Kalle Valo 
> Date: Thu, 19 Jan 2017 20:08:30 +0200
>
>> "John W. Linville"  writes:
>> 
>>> I forgot to Cc Johannes and Kalle...
>> 
>> Also adding linux-wireless.
>> 
>>> On Thu, Jan 19, 2017 at 09:15:09AM -0500, John W. Linville wrote:
>>>
 I'm responsible for this mess. The original idea was for various
 mac80211-based drivers to override the ethtool operation and provide
 their own dump operation, but the mac80211 crowd never embraced
 the idea.
 
 In the meantime, I added the default implementation which just
 passed-up wdev->wiphy->hw_version as the version info for a 0-length
 register dump. I then implemented a driver-specific regiser dump
 handler for userland ethtool that would interpret the hardware version
 information for the at76c50x-usb driver.
 
 So the net of it is, if we treat a return of 0 from get_regs_len()
 as "not supported", we break this one driver-specific feature for
 userland ethtool. Realistically, there are probably very few users
 to care. But I can't guarantee that the number is zero.
>> 
>> I know the number is not zero, because I remember using it years back
>> with something else than at76c50x-usb. But is the number more than one,
>> I don't know :)
>
> I'm trying to dig down and figure out why this problem is showing up now.
> ethtool_get_regs() has been using vzalloc() since 2011, and before that it
> used plain vmalloc().
>
> This code has therefore been using v{m,z}alloc() forever.  What changed?
>
> The zero size check has been in the vmalloc implementation since at least
> 2009.
>
> I don't understand why this is all triggering and being noticed now.  The
> whole ieee80211 "return zero length regs and return hw version in get_regs"
> thing should have been failing for at least 7 years now.

Maybe just nobody hasn't used it since? If my memory serves me right
(too often it does not) It's 6-7 years since I used this, and if the
kernel I worked on at the time was a year or two old, I might have used
a version without the zero size check.

But I'm just hand-waving here, I cannot be sure what's the last kernel I
used.

-- 
Kalle Valo

Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

[David Miller]

From: Kalle Valo 
Date: Thu, 19 Jan 2017 20:08:30 +0200

> "John W. Linville"  writes:
> 
>> I forgot to Cc Johannes and Kalle...
> 
> Also adding linux-wireless.
> 
>> On Thu, Jan 19, 2017 at 09:15:09AM -0500, John W. Linville wrote:
>>> On Thu, Jan 19, 2017 at 07:35:22AM -0500, David Arcari wrote:
>>> > On 01/18/2017 11:45 AM, David Miller wrote:
>>> > > From: David Arcari 
>>> > > Date: Wed, 18 Jan 2017 08:34:05 -0500
>>> > >
>>> > >> If the user executes 'ethtool -d' for an interface and the associated
>>> > >> get_regs_len() function returns 0, the user will see a call trace from
>>> > >> the vmalloc() call in ethtool_get_regs().  This patch modifies
>>> > >> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
>>> > >>
>>> > >> Signed-off-by: David Arcari 
>>> > > I think when the driver indicates this, it is equivalent to saying that
>>> > > the operation isn't supported.
>>> > >
>>> > > Also, this guards us against ->get_regs() methods that don't handle
>>> > > zero length requests properly.  I see many which are going to do
>>> > > really terrible things in that situation.
>>> > >
>>> > > Therefore, if get_regs_len() returns zero, treat it the safe as if the
>>> > > ethtool operations were NULL.
>>> > >
>>> > > Thanks.
>>> > 
>>> > That was actually the fix that I was originally considering, but it
>>> > turns out
>>> > there is a problem with it.
>>> > 
>>> > I found that the vmalloc error was occurring because
>>> > ieee80211_get_regs_len() in
>>> > net/mac80211/ethtool.c was returning zero.  The ieee80211_get_regs in
>>> > the same
>>> > file returns the hw version. It turns out that this information is used
>>> > by the
>>> > at76c50x-usb driver in the user space ethtool to report which HW variant
>>> > is in
>>> > use.  Returning an error when regs_len() returns zero would break this
>>> > functionality.
>>> > 
>>> > -Dave
>>> 
>>> I'm responsible for this mess. The original idea was for various
>>> mac80211-based drivers to override the ethtool operation and provide
>>> their own dump operation, but the mac80211 crowd never embraced
>>> the idea.
>>> 
>>> In the meantime, I added the default implementation which just
>>> passed-up wdev->wiphy->hw_version as the version info for a 0-length
>>> register dump. I then implemented a driver-specific regiser dump
>>> handler for userland ethtool that would interpret the hardware version
>>> information for the at76c50x-usb driver.
>>> 
>>> So the net of it is, if we treat a return of 0 from get_regs_len()
>>> as "not supported", we break this one driver-specific feature for
>>> userland ethtool. Realistically, there are probably very few users
>>> to care. But I can't guarantee that the number is zero.
> 
> I know the number is not zero, because I remember using it years back
> with something else than at76c50x-usb. But is the number more than one,
> I don't know :)

I'm trying to dig down and figure out why this problem is showing up now.
ethtool_get_regs() has been using vzalloc() since 2011, and before that it
used plain vmalloc().

This code has therefore been using v{m,z}alloc() forever.  What changed?

The zero size check has been in the vmalloc implementation since at least
2009.

I don't understand why this is all triggering and being noticed now.  The
whole ieee80211 "return zero length regs and return hw version in get_regs"
thing should have been failing for at least 7 years now.

Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

[Kalle Valo]

"John W. Linville"  writes:

> I forgot to Cc Johannes and Kalle...

Also adding linux-wireless.

> On Thu, Jan 19, 2017 at 09:15:09AM -0500, John W. Linville wrote:
>> On Thu, Jan 19, 2017 at 07:35:22AM -0500, David Arcari wrote:
>> > On 01/18/2017 11:45 AM, David Miller wrote:
>> > > From: David Arcari 
>> > > Date: Wed, 18 Jan 2017 08:34:05 -0500
>> > >
>> > >> If the user executes 'ethtool -d' for an interface and the associated
>> > >> get_regs_len() function returns 0, the user will see a call trace from
>> > >> the vmalloc() call in ethtool_get_regs().  This patch modifies
>> > >> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
>> > >>
>> > >> Signed-off-by: David Arcari 
>> > > I think when the driver indicates this, it is equivalent to saying that
>> > > the operation isn't supported.
>> > >
>> > > Also, this guards us against ->get_regs() methods that don't handle
>> > > zero length requests properly.  I see many which are going to do
>> > > really terrible things in that situation.
>> > >
>> > > Therefore, if get_regs_len() returns zero, treat it the safe as if the
>> > > ethtool operations were NULL.
>> > >
>> > > Thanks.
>> > 
>> > That was actually the fix that I was originally considering, but it
>> > turns out
>> > there is a problem with it.
>> > 
>> > I found that the vmalloc error was occurring because
>> > ieee80211_get_regs_len() in
>> > net/mac80211/ethtool.c was returning zero.  The ieee80211_get_regs in
>> > the same
>> > file returns the hw version. It turns out that this information is used
>> > by the
>> > at76c50x-usb driver in the user space ethtool to report which HW variant
>> > is in
>> > use.  Returning an error when regs_len() returns zero would break this
>> > functionality.
>> > 
>> > -Dave
>> 
>> I'm responsible for this mess. The original idea was for various
>> mac80211-based drivers to override the ethtool operation and provide
>> their own dump operation, but the mac80211 crowd never embraced
>> the idea.
>> 
>> In the meantime, I added the default implementation which just
>> passed-up wdev->wiphy->hw_version as the version info for a 0-length
>> register dump. I then implemented a driver-specific regiser dump
>> handler for userland ethtool that would interpret the hardware version
>> information for the at76c50x-usb driver.
>> 
>> So the net of it is, if we treat a return of 0 from get_regs_len()
>> as "not supported", we break this one driver-specific feature for
>> userland ethtool. Realistically, there are probably very few users
>> to care. But I can't guarantee that the number is zero.

I know the number is not zero, because I remember using it years back
with something else than at76c50x-usb. But is the number more than one,
I don't know :)

>> Possible solutions:
>> 
>>  -- break userland ethtool for at76c50x-usb
>>  -- avoid 0-len allocation attempt (David Arcari's patch)
>>  -- make allocator accept a 0 length value w/o oops'ing
>>  -- change mac8011 code to return non-zero from get_regs_len()
>> 
>> Thoughts? The last option holds a certain attraction, but I'm not
>> sure how to make it useful...?

If it were only about at76c50x-usb I would say go for it, nobody sane
should use that device anymore. But on the other hand I'm worried that
this interface might be used by other (proprietary) user space tools
with other, more important, drivers. A difficult situation.

-- 
Kalle Valo

Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

[John W. Linville]

I forgot to Cc Johannes and Kalle...

On Thu, Jan 19, 2017 at 09:15:09AM -0500, John W. Linville wrote:
> On Thu, Jan 19, 2017 at 07:35:22AM -0500, David Arcari wrote:
> > On 01/18/2017 11:45 AM, David Miller wrote:
> > > From: David Arcari 
> > > Date: Wed, 18 Jan 2017 08:34:05 -0500
> > >
> > >> If the user executes 'ethtool -d' for an interface and the associated
> > >> get_regs_len() function returns 0, the user will see a call trace from
> > >> the vmalloc() call in ethtool_get_regs().  This patch modifies
> > >> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
> > >>
> > >> Signed-off-by: David Arcari 
> > > I think when the driver indicates this, it is equivalent to saying that
> > > the operation isn't supported.
> > >
> > > Also, this guards us against ->get_regs() methods that don't handle
> > > zero length requests properly.  I see many which are going to do
> > > really terrible things in that situation.
> > >
> > > Therefore, if get_regs_len() returns zero, treat it the safe as if the
> > > ethtool operations were NULL.
> > >
> > > Thanks.
> > 
> > That was actually the fix that I was originally considering, but it
> > turns out
> > there is a problem with it.
> > 
> > I found that the vmalloc error was occurring because
> > ieee80211_get_regs_len() in
> > net/mac80211/ethtool.c was returning zero.  The ieee80211_get_regs in
> > the same
> > file returns the hw version. It turns out that this information is used
> > by the
> > at76c50x-usb driver in the user space ethtool to report which HW variant
> > is in
> > use.  Returning an error when regs_len() returns zero would break this
> > functionality.
> > 
> > -Dave
> 
> I'm responsible for this mess. The original idea was for various
> mac80211-based drivers to override the ethtool operation and provide
> their own dump operation, but the mac80211 crowd never embraced
> the idea.
> 
> In the meantime, I added the default implementation which just
> passed-up wdev->wiphy->hw_version as the version info for a 0-length
> register dump. I then implemented a driver-specific regiser dump
> handler for userland ethtool that would interpret the hardware version
> information for the at76c50x-usb driver.
> 
> So the net of it is, if we treat a return of 0 from get_regs_len()
> as "not supported", we break this one driver-specific feature for
> userland ethtool. Realistically, there are probably very few users
> to care. But I can't guarantee that the number is zero.
> 
> Possible solutions:
> 
>   -- break userland ethtool for at76c50x-usb
>   -- avoid 0-len allocation attempt (David Arcari's patch)
>   -- make allocator accept a 0 length value w/o oops'ing
>   -- change mac8011 code to return non-zero from get_regs_len()
> 
> Thoughts? The last option holds a certain attraction, but I'm not
> sure how to make it useful...?
> 
> John
> 
> -- 
> John W. Linville  Someday the world will need a hero, and you
> linvi...@tuxdriver.commight be all we have.  Be ready.
> 

-- 
John W. LinvilleSomeday the world will need a hero, and you
linvi...@tuxdriver.com  might be all we have.  Be ready.

Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

[John W. Linville]

On Thu, Jan 19, 2017 at 07:35:22AM -0500, David Arcari wrote:
> On 01/18/2017 11:45 AM, David Miller wrote:
> > From: David Arcari 
> > Date: Wed, 18 Jan 2017 08:34:05 -0500
> >
> >> If the user executes 'ethtool -d' for an interface and the associated
> >> get_regs_len() function returns 0, the user will see a call trace from
> >> the vmalloc() call in ethtool_get_regs().  This patch modifies
> >> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
> >>
> >> Signed-off-by: David Arcari 
> > I think when the driver indicates this, it is equivalent to saying that
> > the operation isn't supported.
> >
> > Also, this guards us against ->get_regs() methods that don't handle
> > zero length requests properly.  I see many which are going to do
> > really terrible things in that situation.
> >
> > Therefore, if get_regs_len() returns zero, treat it the safe as if the
> > ethtool operations were NULL.
> >
> > Thanks.
> 
> That was actually the fix that I was originally considering, but it
> turns out
> there is a problem with it.
> 
> I found that the vmalloc error was occurring because
> ieee80211_get_regs_len() in
> net/mac80211/ethtool.c was returning zero.  The ieee80211_get_regs in
> the same
> file returns the hw version. It turns out that this information is used
> by the
> at76c50x-usb driver in the user space ethtool to report which HW variant
> is in
> use.  Returning an error when regs_len() returns zero would break this
> functionality.
> 
> -Dave

I'm responsible for this mess. The original idea was for various
mac80211-based drivers to override the ethtool operation and provide
their own dump operation, but the mac80211 crowd never embraced
the idea.

In the meantime, I added the default implementation which just
passed-up wdev->wiphy->hw_version as the version info for a 0-length
register dump. I then implemented a driver-specific regiser dump
handler for userland ethtool that would interpret the hardware version
information for the at76c50x-usb driver.

So the net of it is, if we treat a return of 0 from get_regs_len()
as "not supported", we break this one driver-specific feature for
userland ethtool. Realistically, there are probably very few users
to care. But I can't guarantee that the number is zero.

Possible solutions:

-- break userland ethtool for at76c50x-usb
-- avoid 0-len allocation attempt (David Arcari's patch)
-- make allocator accept a 0 length value w/o oops'ing
-- change mac8011 code to return non-zero from get_regs_len()

Thoughts? The last option holds a certain attraction, but I'm not
sure how to make it useful...?

John

-- 
John W. LinvilleSomeday the world will need a hero, and you
linvi...@tuxdriver.com  might be all we have.  Be ready.

Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

[David Arcari]

On 01/18/2017 11:45 AM, David Miller wrote:
> From: David Arcari 
> Date: Wed, 18 Jan 2017 08:34:05 -0500
>
>> If the user executes 'ethtool -d' for an interface and the associated
>> get_regs_len() function returns 0, the user will see a call trace from
>> the vmalloc() call in ethtool_get_regs().  This patch modifies
>> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
>>
>> Signed-off-by: David Arcari 
> I think when the driver indicates this, it is equivalent to saying that
> the operation isn't supported.
>
> Also, this guards us against ->get_regs() methods that don't handle
> zero length requests properly.  I see many which are going to do
> really terrible things in that situation.
>
> Therefore, if get_regs_len() returns zero, treat it the safe as if the
> ethtool operations were NULL.
>
> Thanks.

That was actually the fix that I was originally considering, but it
turns out
there is a problem with it.

I found that the vmalloc error was occurring because
ieee80211_get_regs_len() in
net/mac80211/ethtool.c was returning zero.  The ieee80211_get_regs in
the same
file returns the hw version. It turns out that this information is used
by the
at76c50x-usb driver in the user space ethtool to report which HW variant
is in
use.  Returning an error when regs_len() returns zero would break this
functionality.

-Dave

Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

[David Miller]

From: David Arcari 
Date: Wed, 18 Jan 2017 08:34:05 -0500

> If the user executes 'ethtool -d' for an interface and the associated
> get_regs_len() function returns 0, the user will see a call trace from
> the vmalloc() call in ethtool_get_regs().  This patch modifies
> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
> 
> Signed-off-by: David Arcari 

I think when the driver indicates this, it is equivalent to saying that
the operation isn't supported.

Also, this guards us against ->get_regs() methods that don't handle
zero length requests properly.  I see many which are going to do
really terrible things in that situation.

Therefore, if get_regs_len() returns zero, treat it the safe as if the
ethtool operations were NULL.

Thanks.

[PATCH] net: ethtool: avoid allocation failure for dump_regs

[David Arcari]

If the user executes 'ethtool -d' for an interface and the associated
get_regs_len() function returns 0, the user will see a call trace from
the vmalloc() call in ethtool_get_regs().  This patch modifies
ethtool_get_regs() to avoid the call to vmalloc when the size is zero.

Signed-off-by: David Arcari 
---
 net/core/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index e23766c..47acd6f 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1405,7 +1405,7 @@ static int ethtool_get_regs(struct net_device *dev, char 
__user *useraddr)
if (regs.len > reglen)
regs.len = reglen;
 
-   regbuf = vzalloc(reglen);
+   regbuf = reglen ? vzalloc(reglen) : NULL;
if (reglen && !regbuf)
return -ENOMEM;
 
--