netdev - Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp

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]

Date: Mon, 4 Dec 2023 20:45:32 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "Austin, Alex (DCCG)" <alexaust@....com>,
	Alex Austin <alex.austin@....com>, netdev@...r.kernel.org,
	linux-net-drivers@....com, ecree.xilinx@...il.com,
	habetsm.xilinx@...il.com, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, richardcochran@...il.com, lorenzo@...nel.org,
	memxor@...il.com, alardam@...il.com, bhelgaas@...gle.com
Subject: Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

On Mon, Dec 04, 2023 at 10:17:05AM -0800, Jakub Kicinski wrote:
> On Mon, 4 Dec 2023 13:00:35 +0200 Vladimir Oltean wrote:
> > If I may intervene. The "request state" will ultimately go away once all
> > drivers are converted. I know it's more fragile and not all fields are
> > valid, but I think I would like drivers to store the kernel_ variant of
> > the structure, because more stuff will be added to the kernel_ variant
> > in the future (the hwtstamp provider + qualifier), and doing this from
> > the beginning will avoid reworking them again.
> 
> Okay, you know the direction of this work better, so:
> 
> pw-bot: under-review

I mean your observation is in principle fair. If drivers save the struct
kernel_hwtstamp_config in the set() method and give it back in the get()
method (this is very widespread BTW), it's reasonable to question what
happens with the temporary fields, ifr and copied_to_user. Won't we
corrupt the teporary fields of the kernel_hwtstamp_config structure from
the set() with the previous ones from the get()?

The answer, I think, is that we do, but in a safe way. Because we implement
ndo_hwtstamp_set(), the copied_to_user that we save is false (aka "the
driver implementation didn't call copy_to_user()"). And when we give
this structure back in ndo_hwtstamp_get(), we overwrite false with false,
and a good ifr pointer with a bad one.

But the only reason we transport the ifr along with the
kernel_hwtstamp_config is for generic_hwtstamp_ioctl_lower() to work,
aka a new API upper driver on top of an old API real driver. Which is
not the case here, and no one looks at the stale ifr pointer.

It's a lot to think about to make sure that something bad won't happen,
I agree. I still don't believe it will break in subtle ways, but nonetheless
I do recognize the tradeoff. One approach is more straightforward
code-wise but more subtle behavior-wise, and the other is the opposite.

> 
> Report-bugs-to: Vladimir Oltean <olteanv@...il.com>
> 
> :P

Hmm, I'm not sure if I want to go that far. Alex is free to choose
whichever implementation he sees fit, and so, he is also responsible
for the end result, in spite of any review feedback received. Please
don't consider my message as anything more than a suggestion.