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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ