Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 2 Jun 2023 22:55:19 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Ding Hui <dinghui@sangfor.com.cn>
Cc:     Alexander Duyck <alexander.duyck@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>, davem@davemloft.net,
        edumazet@google.com, pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, pengdonglin@sangfor.com.cn,
        huangcun@sangfor.com.cn
Subject: Re: [PATCH net-next] net: ethtool: Fix out-of-bounds copy to user
Message-ID: <20230602225519.66c2c987@kernel.org>
In-Reply-To: <44905acd-3ac4-cfe5-5e91-d182c1959407@sangfor.com.cn>
References: <20230601112839.13799-1-dinghui@sangfor.com.cn>
        <135a45b2c388fbaf9db4620cb01b95230709b9ac.camel@gmail.com>
        <eed0cbf7-ff12-057e-e133-0ddf5e98ef68@sangfor.com.cn>
        <6110cf9f-c10e-4b9b-934d-8d202b7f5794@lunn.ch>
        <f7e23fe6-4d30-ef1b-a431-3ef6ec6f77ba@sangfor.com.cn>
        <6e28cea9-d615-449d-9c68-aa155efc8444@lunn.ch>
        <CAKgT0UdyykQL-BidjaNpjX99FwJTxET51U29q4_CDqmABUuVbw@mail.gmail.com>
        <ece228a3-5c31-4390-b6ba-ec3f2b6c5dcb@lunn.ch>
        <CAKgT0Uf+XaKCFgBRTn-viVsKkNE7piAuDpht=efixsAV=3JdFQ@mail.gmail.com>
        <44905acd-3ac4-cfe5-5e91-d182c1959407@sangfor.com.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 3 Jun 2023 09:51:34 +0800 Ding Hui wrote:
> > If that is the case maybe it would just make more sense to just return
> > an error if we are at risk of overrunning the userspace allocated
> > buffer.
> 
> In that case, I can modify to return an error, however, I think the
> ENOSPC or EFBIG mentioned in a previous email may not be suitable,
> maybe like others length/size checking return EINVAL.
> 
> Another thing I wondered is that should I update the current length
> back to user if user buffer is not enough, assuming we update the new
> length with error returned, the userspace can use it to reallocate
> buffer if he wants to, which can avoid re-call previous ioctl to get
> the new length.

This entire thread presupposes that user provides the length of 
the buffer. I don't see that in the code. Take ethtool_get_stats()
as an example, you assume that stats.n_stats is set correctly,
but it's not enforced today. Some app somewhere may pass in zeroed
out stats and work just fine.