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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5f0f2bab-ae36-8b13-2c6d-c69c6ff4a43f@sangfor.com.cn>
Date:   Sat, 3 Jun 2023 15:11:29 +0800
From:   Ding Hui <dinghui@...gfor.com.cn>
To:     Jakub Kicinski <kuba@...nel.org>
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 2023/6/3 13:55, Jakub Kicinski wrote:
> 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.
> 

Yes.

I checked the others ioctl (e.g. ethtool_get_eeprom(), ethtool_get_features()),
and searched the git log of ethtool utility, so I think that is an implicit
rule and the check is missed in kernel where the patch involves.

Without this rule, we cannot guarantee the safety of copy to user.

Should we keep to be compatible with that incorrect userspace usage?

-- 
Thanks,
- Ding Hui