Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_set_locality()

[Jarkko Sakkinen]

On Wed, 2023-05-31 at 19:32 +0300, Jarkko Sakkinen wrote:
> On Wed, 2023-05-31 at 11:20 -0400, Stefan Berger wrote:
> > 
> > On 5/30/23 16:50, Jarkko Sakkinen wrote:
> > > From: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxx>
> > > 
> > > vtpm_proxy_fops_set_locality() causes kernel buffers to be passed to
> > > copy_from_user() and copy_to_user().
> > 
> > And what is the problem with that? Is it not working?
> 
> It is API contract and also clearly documented in the kernel documentation.
> 
> This should be obvious even if you have've consulted that documentation because
> both functions have 'user' suffix, and also the pointer is __user tagged.
> 
> To make things worse it is architecture specific. I'm worried that it will
> break in one of the 23 microarchitectures. Have you actually ever checked it
> does not?
> 
> I'm not also an expert of how all the possible CPUs in the world empower
> Linux to further restrict the move between different memory spaces. I'm
> quite sure that this does conflict neither with SMAP or SMEP on x86
> (because I know x86 pretty well), but who knows what they add in the
> future to the microarchitecture.
> 
> > > Factor out the crippled code away with help of an internal API for
> > > managing struct proxy_dev instances.
> > 
> > What is crippled code?
> 
> Code that behaves badly, i.e. does not meat the expectations. Illegit use of
> in-kernel functions easily fits to the definition of crippled code.
> 
> Bad API behavior put aside, it is very inefficient implementation because it
> unnecessarily recurses tpm_transmit(), which makes extending the driver to
> any direction so much involved process, but we don't really need this as a
> rationale.
> 
> This needs to be fixed in a way or another. That is dictated by the API
> cotract so for that I do not really even need feedback because it is
> force majeure. I'm cool with alternatives or suggestions to the current
> fact, so please focus on that instead of asking question that kernel
> documentation provides you already all the answers.

I have to add that it has not been too critical because afaik tpm_vtpm_proxy
is for the most part for development use, and less of so production. This is
just to say that overally we've been happy with the driver in its use cases
but any snippet of code always has an expiration date.

BR, Jarkko