ethtool 5.8 segfaults when changing settings on a device

ethtool 5.8 segfaults when changing settings on a device

[Hans-Christian Egtvedt (hegtvedt)]

Hello,

I am testing ethtool 5.8, and I noticed it segfaulted with the command
   ethtool -s eth0 autoneg on

Backtrace as follows:
(gdb) run -s eth0 autoneg on
Starting program: /tmp/ethtool-5.8 -s eth0 autoneg on

Program received signal SIGSEGV, Segmentation fault.
0x0040bef8 in do_sset ()
(gdb) bt
#0  0x0040bef8 in do_sset ()
#1  0x00407d9c in do_ioctl_glinksettings ()
#2  0x00000000 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

I then tested reverting 
https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/ethtool.c?id=bef780467fa7aa95dca3ed5cc3ebb8bec5882f08

And the command now passes.

I am running ethtool on top of Linux 4.4.232, hence there is no support 
for ETHTOOL_xLINKSETTINGS.

Is the bef780467fa7aa95dca3ed5cc3ebb8bec5882f08 patch not correct, one 
should check link_usettings pointer for non-NULL before memset'ing?

Since do_ioctl_glinksettings() will return NULL upon failure, which 
matches well with kernels not supporting ETHTOOL_GLINKSETTINGS ioctl.

-- 
Best regards, Hans-Christian Noren Egtvedt

Re: ethtool 5.8 segfaults when changing settings on a device

[Michal Kubecek]

On Thu, Aug 13, 2020 at 12:25:22PM +0000, Hans-Christian Egtvedt (hegtvedt) wrote:
> Hello,
> 
> I am testing ethtool 5.8, and I noticed it segfaulted with the command
>    ethtool -s eth0 autoneg on
> 
> Backtrace as follows:
> (gdb) run -s eth0 autoneg on
> Starting program: /tmp/ethtool-5.8 -s eth0 autoneg on
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0040bef8 in do_sset ()
> (gdb) bt
> #0  0x0040bef8 in do_sset ()
> #1  0x00407d9c in do_ioctl_glinksettings ()
> #2  0x00000000 in ?? ()
> Backtrace stopped: previous frame identical to this frame (corrupt stack?)
> 
> I then tested reverting 
> https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/ethtool.c?id=bef780467fa7aa95dca3ed5cc3ebb8bec5882f08
> 
> And the command now passes.
> 
> I am running ethtool on top of Linux 4.4.232, hence there is no support 
> for ETHTOOL_xLINKSETTINGS.
> 
> Is the bef780467fa7aa95dca3ed5cc3ebb8bec5882f08 patch not correct, one 
> should check link_usettings pointer for non-NULL before memset'ing?
> 
> Since do_ioctl_glinksettings() will return NULL upon failure, which 
> matches well with kernels not supporting ETHTOOL_GLINKSETTINGS ioctl.

Thank you for the report. Reverting commit bef780467fa7 would bring back
the issue it fixed. AFAICS the only problem is indeed the missing null
check which allows dereferencing the pointer returned by
do_ioctl_glinksettings() even if it is null. I didn't realize the
pointer can be null which is rather shameful as there is a null check
right below the inserted memset(). Thus adding the null check (which can
be combined with one that is already there) should be the easiest way to
fix this.

Please feel free to submit the fix.

Michal