linux-kernel - Re: [PATCH] tpm: factor out the user space mm from tpm_vtpm_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] [day] [month] [year] [list]

Date:   Thu, 08 Jun 2023 21:59:46 +0300
From:   "Jarkko Sakkinen" <jarkko@...nel.org>
To:     "Stefan Berger" <stefanb@...ux.ibm.com>,
        <linux-integrity@...r.kernel.org>
Cc:     "Jason Gunthorpe" <jgg@...dia.com>,
        "Alejandro Cabrera" <alejandro.cabreraaldaya@...i.fi>,
        "Jarkko Sakkinen" <jarkko.sakkinen@...i.fi>,
        <stable@...r.kernel.org>,
        "Stefan Berger" <stefanb@...ux.vnet.ibm.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tpm: factor out the user space mm from
 tpm_vtpm_set_locality()

On Thu Jun 8, 2023 at 6:10 PM EEST, Stefan Berger wrote:
>
>
> On 6/8/23 09:14, Jarkko Sakkinen wrote:
> > On Wed May 31, 2023 at 8:01 PM EEST, Stefan Berger wrote:
> >>
> >>
> >
> >>
> >> This is swtpm picking up this command with its user buffer.
> >>
> >>     So, I am not sure at this point what is wrong.
> >>
> >>      Stefan
> > 
> > The answer was below but in short it is that you have a function that
> > expects __user * and you don't pass user tagged memory.
>
> There are two functions that expect user tagged memory:
>
> static ssize_t vtpm_proxy_fops_read(struct file *filp, char __user *buf,
> 				    size_t count, loff_t *off)
> static ssize_t vtpm_proxy_fops_write(struct file *filp, const char __user *buf,
> 				     size_t count, loff_t *off)
>
> the correspond to this interface:
>
> struct file_operations {
> 	struct module *owner;
> 	loff_t (*llseek) (struct file *, loff_t, int);
> 	ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
> 	ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
>
> defined here:
>
> static const struct file_operations vtpm_proxy_fops = {
> 	.owner = THIS_MODULE,
> 	.llseek = no_llseek,
> 	.read = vtpm_proxy_fops_read,
> 	.write = vtpm_proxy_fops_write,
>
> Conversely, I see no other function interfaces in tpm_vtpm_proxy.c where the code would be missing the __user.
>
> Neither do I see any functions where I am passing a __user tagged buffer as parameter that shouldn't have
> such a tag on it or the reverse where a plain buffer is passed and it should be a __user tagged buffer.

Uh oh, you're correct. I was looking this in the context of my user
vtpm driver changes, so that confused me, so it is actually my bad.
So blame is on my side.

I would still want to open code the whole thing because there is no
need to cycle it through tpm_transmit_cmd() but I'll do it in the
context of other changes.

It is pretty complicated sequence and makes hard to build anything
on top of existing functionality, so it needs to be simplified in
any case... But you are right: it is not a bug.

BR, Jarkko