Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 01/19/2016 
06:51:07 PM:

> 
> On Thu, Jan 14, 2016 at 11:01:57AM -0500, Stefan Berger wrote:
> > +   pdev = platform_device_register_simple("tpm_vtpm", 
vtpm_dev->dev_num,
> > +                      NULL, 0);
> 
> This seems strange, something like this should not be creating
> platform_devices.

Should it be a misc_device then ?

> 
> tpm's cannot be attached to platform_devices outside their probe
> functions, I just fixed that bug in tpm_tis.
> 
> Attach the tpm to a device on the vtpm_class? Do it through a callback
> that takes care of devm? IIRC there is one but the name escapes me now
> :)
> 
> > +   vtpm_dev->chip = tpmm_chip_alloc_pattern(vtpm_dev->pdev,
> > +            &vtpm_tpm_ops,
> > +            VTPM_DEV_PREFIX_CLIENT"%d");
> 
> I agree with Jarkko I don't see why these deserve special names..

I did that because it becomes confusing when there are many /dev/tpm%d 
type of devices. This way we can distinguish between a hardware /dev/tpm%d 
and all these types of devices.

> 
> Presumably some namespace magic can be used to make them show up as
> tpm0 in a container?

The magic is to have the container management stack create the device 
pair. From the ioctl it learns the name of the devices that were created 
and it then finds out about the major/minor number of the created device 
and have /dev/tpm0 with that major/minor created in the container's /dev 
directory.

> 
> > +   vtpm_dev->chip->flags |= TPM_CHIP_FLAG_NO_SYSFS | 
TPM_CHIP_FLAG_NO_LOG;
> 
> Why no sysfs files?

I have changed this now to the following

+       if (vtpm_dev->flags & VTPM_FLAG_NO_SYSFS)
+               vtpm_dev->chip->flags |= TPM_CHIP_FLAG_NO_SYSFS;
+
+       if (vtpm_dev->flags & VTPM_FLAG_NO_LOG)
+               vtpm_dev->chip->flags |= TPM_CHIP_FLAG_NO_LOG;
+

https://github.com/stefanberger/linux/commit/5dd68fef872f10261521c91c23b1d0dfbdf69996#diff-0d6b7dbf28fbfdd13909c2e1b5a6d7a3R541


The issue with sysfs is that sysfs is showing directory entries for all 
vTPMs in all containers. If we don't want users to see the PCRs of other 
containers we set this flag. The side effect of setting this flag is that 
the container will not be able to see its own TPM device, either, but 
presumably the user has commands to reads PCRs etc.

Regarding VTPM_FLAG_NO_LOG: Container's boot/start without firmware that 
would write an ACPI log. So in most cases one will choose the 
VTPM_FLAG_NO_LOG flag since there is no log. Maybe that's a flag the user 
shouldn't have control over.

> 
> > +   switch (ioctl) {
> > +   case VTPM_NEW_DEV:
> > +      if (!capable(CAP_SYS_ADMIN))
> > +         return -EPERM;
> > +      vtpm_new_pair_p = argp;
> > +      if (copy_from_user(&vtpm_new_pair, vtpm_new_pair_p,
> > +               sizeof(vtpm_new_pair)))
> > +         return -EFAULT;
> > +      vtpm_dev = vtpm_create_device_pair(&vtpm_new_pair);
> > +      if (IS_ERR(vtpm_dev))
> > +         return PTR_ERR(vtpm_dev);
> > +      if (copy_to_user(vtpm_new_pair_p, &vtpm_new_pair,
> > +             sizeof(vtpm_new_pair)))
> > +         return -EFAULT;
> 
> Another way to handle this is to return the FD for the server side of
> the new TPM here and then route accesses from the /dev/tpmX side to
> that FD. Avoid the extra char device. There is some kernel
> infrasturcture that helpd do this (anon fd or something, IIRC)

Hm, I would prefer to keep the server-side char device since that way one 
isn't tied to fd passing or so and the TPM could terminate and be 
restarted later on on top of that server side device while keeping the 
client side device usable beyond the server side shutting down.

> 
> For this usage that might make more sense than trying to copy the ptmx
> scheme, which is like that for other reasons..

:-) Yes, ptmx is where I took this from.

> 
> > +   case VTPM_DEL_DEV:
> > +      if (!capable(CAP_SYS_ADMIN))
> > +         return -EPERM;
> 
> Then del becomes close() on the special fd

And with that the client side device would *have* to close as well because 
the backend now is inaccessible?

> 
> > +   case VTPM_GET_VTPMDEV:
> 
> And this wouldn't be needed

Assuming we want to keep server side for longer time, this ioctl and 
VTPM_GET_TPMDEV help to determine the other side of device that found in 
/dev.

Thanks for looking at the patches. I would prefer to keep the two 
character devices rather than the server side only being represented with 
an fd.

   Stefan

> 
> Jason
>