netdev - Re: ethtool 5.8 segfaults when changing settings on a device

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] [day] [month] [year] [list]

Date:   Sun, 23 Aug 2020 22:16:23 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     "Hans-Christian Egtvedt (hegtvedt)" <hegtvedt@...co.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: ethtool 5.8 segfaults when changing settings on a device

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