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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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....