linux-kernel - Re: [PATCH net-next] net: ethtool: Fix out-of-bounds copy to user

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230602225519.66c2c987@kernel.org>
Date:   Fri, 2 Jun 2023 22:55:19 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ding Hui <dinghui@...gfor.com.cn>
Cc:     Alexander Duyck <alexander.duyck@...il.com>,
        Andrew Lunn <andrew@...n.ch>, davem@...emloft.net,
        edumazet@...gle.com, pabeni@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, pengdonglin@...gfor.com.cn,
        huangcun@...gfor.com.cn
Subject: Re: [PATCH net-next] net: ethtool: Fix out-of-bounds copy to user

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.