netdev - Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

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: <20230401182058.zt5qhgjmejm7lnst@skbuf>
Date:   Sat, 1 Apr 2023 21:20:58 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Maxim Georgiev <glipus@...il.com>, kory.maincent@...tlin.com,
        netdev@...r.kernel.org, maxime.chevallier@...tlin.com
Subject: Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

On Sat, Apr 01, 2023 at 10:55:33AM -0700, Jakub Kicinski wrote:
> On Sat, 1 Apr 2023 19:08:29 +0300 Vladimir Oltean wrote:
> > > Let's refactor this differently, we need net_hwtstamp_validate()
> > > to run on the same in-kernel copy as we'll send down to the driver.
> > > If we copy_from_user() twice we may validate a different thing
> > > than the driver will end up seeing (ToCToU).  
> > 
> > I'm not sure I understand this. Since net_hwtstamp_validate() already
> > contains a copy_from_user() call, don't we already call copy_to_user()
> > twice (the second time being in all SIOCSHWTSTAMP handlers from drivers)?
> 
> After this patch we'll be passing an in-kernel-space struct to drivers
> rather than the ifr they have to copy themselves. I'm saying that we
> should validate that exact copy, rather than copy, validate, copy, pass
> to drivers, cause user space may change the values between the two
> copies.
> 
> Unlikely to cause serious bugs but seems like a good code hygiene.
> 
> This is only for the drivers converted to the NDO, obviously, 
> the legacy drivers will still have to copy themselves.

Could you answer my second paragraph too, please?

| Perhaps I don't understand what is it that can change the contents
| of the ifreq structure, which would make this a potential issue for
| ndo_hwtstamp_set() that isn't an issue for ndo_eth_ioctl()...

I don't disagree with minimizing the number of copy_to_user() calls, but
I don't understand the ToCToU argument that you're bringing....