Re: [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Jan 11, 2022 at 3:29 PM Andrew Cooper <amc96@xxxxxxxx> wrote:
>
> On 11/01/2022 15:12, Juergen Gross wrote:
> > diff --git a/tpm_tis.c b/tpm_tis.c
> > index 477f555..abea7a1 100644
> > --- a/tpm_tis.c
> > +++ b/tpm_tis.c
> > @@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip, uint32_t 
> > subcap_id, cap_t *cap,
> >          return rc;
> >  }
> >
> > +static void shutdown_tpm_tis(struct tpm_chip* tpm){
>
> Style, as you're moving it.  Also in the function.
>
> > @@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat* buf)
> >     return 0;
> >  }
> >
> > +static struct file_ops tpm_tis_ops = {
> > +    .name = "tpm_tis",
> > +    .read = tpm_tis_posix_read,
> > +    .write = tpm_tis_posix_write,
> > +    .lseek = lseek_default,
> > +    .close = tpm_tis_close,
> > +    .fstat = tpm_tis_posix_fstat,
> > +};
> > +
> > +int tpm_tis_open(struct tpm_chip* tpm)
>
> Style.
>
> > +{
> > +   struct file *file;
> > +   static unsigned int ftype_tis;
> > +
> > +   /* Silently prevent multiple opens */
> > +   if(tpm->fd != -1) {
> > +      return tpm->fd;
> > +   }
>
> Another WTF moment.  We silently swallow multiple open()s, but don't
> refcout close()s ?
>
> This cannot be correct, or sensible, behaviour.
>
> Jason/Daniel - thoughts?

Looks like vtpmmgr only opens a single global fd, so it has not been a
problem in practice.

You need some sort of synchronization to let multiple entities access
the TPM.  So limiting to only a single entity/single FD is a
reasonable restriction.

Regards,
Jason