linux-kernel - Re: [PATCH net-next] ethtool: copy reglen to userspace

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]

Message-ID: <20190530193842.GB20549@t480s.localdomain>
Date:   Thu, 30 May 2019 19:38:42 -0400
From:   Vivien Didelot <vivien.didelot@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     mkubecek@...e.cz, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...oirfairelinux.com,
        linville@...hat.com, f.fainelli@...il.com
Subject: Re: [PATCH net-next] ethtool: copy reglen to userspace

Hi Michal, David,

On Thu, 30 May 2019 11:26:30 -0700 (PDT), David Miller <davem@...emloft.net> wrote:
> From: Michal Kubecek <mkubecek@...e.cz>
> Date: Thu, 30 May 2019 10:27:22 +0200
> 
> > I believe this should be handled by ethtool_get_regs(), either by
> > returning an error or by only copying data up to original regs.len
> > passed by userspace. The former seems more correct but broken userspace
> > software would suddenly start to fail where it "used to work". The
> > latter would be closer to current behaviour but it would mean that
> > broken userspace software might nerver notice there is something wrong.
> 
> I therefore think we need to meticulously fixup all of these adjustments
> of regs.len before adjusting the copy call here in generic ethtool.

Indeed there are cases where userspace may allocate a smaller buffer
(even though there's no way for the kernel to ensure the size that
was really allocated, but well...).

The kernel must still allocates a buffer of size ops->get_regs_len()
and pass it to the driver. It's OK if the kernel driver messes with
regs->len, since it's an open structure. Software may actually make
use of it. However the kernel must use the original regs->len passed
by userspace (up to ops->get_regs_len()) when copying the data.

I'm sending a new patch right away.

Thanks,
Vivien