Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 30 May 2019 19:38:42 -0400
Message-ID: <20190530193842.GB20549@t480s.localdomain>
From:   Vivien Didelot <vivien.didelot@gmail.com>
To:     David Miller <davem@davemloft.net>
Cc:     mkubecek@suse.cz, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com,
        linville@redhat.com, f.fainelli@gmail.com
Subject: Re: [PATCH net-next] ethtool: copy reglen to userspace
In-Reply-To: <20190530.112630.476132829981439143.davem@davemloft.net>
References: <20190529.221744.1136074795446305909.davem@davemloft.net>
 <20190530064848.GA27401@unicorn.suse.cz>
 <20190530082722.GB27401@unicorn.suse.cz>
 <20190530.112630.476132829981439143.davem@davemloft.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Michal, David,

On Thu, 30 May 2019 11:26:30 -0700 (PDT), David Miller <davem@davemloft.net> wrote:
> From: Michal Kubecek <mkubecek@suse.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