Re: [PATCH v3 09/11] tpm: Driver for supporting multiple emulated TPMs

["Stefan Berger" <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6119 bytes --]

Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote on 02/23/2016 
05:22:11 AM:


> 
> On Fri, Feb 19, 2016 at 07:42:06AM -0500, Stefan Berger wrote:
> > This patch implements a driver for supporting multiple emulated TPMs 
in a
> > system.
> > 
> > The driver implements a device /dev/vtpmx that is used to created
> > a client device pair /dev/tpmX (e.g., /dev/tpm10) and a server side 
that
> > is accessed using a file descriptor returned by an ioctl.
> > The device /dev/tpmX is the usual TPM device created by the core TPM
> > driver. Applications or kernel subsystems can send TPM commands to it
> > and the corresponding server-side file descriptor receives these
> > commands and delivers them to an emulated TPM.
> > 
> > Signed-off-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/char/tpm/Kconfig    |  10 +
> >  drivers/char/tpm/Makefile   |   1 +
> >  drivers/char/tpm/tpm-vtpm.c | 543 +++++++++++++++++++++++++++++++
> +++++++++++++
> >  include/uapi/linux/Kbuild   |   1 +
> >  include/uapi/linux/vtpm.h   |  38 ++++
> >  5 files changed, 593 insertions(+)
> >  create mode 100644 drivers/char/tpm/tpm-vtpm.c
> >  create mode 100644 include/uapi/linux/vtpm.h
> > 
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index 3b84a8b..4c4e843 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -122,5 +122,15 @@ config TCG_CRB
> >       from within Linux.  To compile this driver as a module, choose
> >       M here; the module will be called tpm_crb.
> > 

> > +
> > +struct vtpm_dev {
> > +   struct tpm_chip *chip;
> > +
> > +   u32 flags;                   /* public API flags */
> > +
> > +   long state;
> > +#define STATE_OPENED_BIT        0
> > +#define STATE_WAIT_RESPONSE_BIT 1    /* waiting for emulator to 
> give response */
> 
> I'd prefer something like this before declaring the struct:
> 
> enum vtpm_dev_states {
>    VTPM_DEV_OPENED         = BIT(0),
>    VTPM_DEV_WAITING_FOR_RESPONSE   = BIT(1),
> };
> 
> This whole use of set/clear_bit macros when you don't have variable
> number of bits just makes code less transparent.

Though read-modify-writes with the bit ops are atomic and need no spinlock 
protection to avoid concurrency mess. Now we would need a spinlock for 
such ops.


> 
> > +
> > +   spinlock_t buf_lock;         /* lock for buffers */
> > +
> > +   wait_queue_head_t wq;
> > +
> > +   size_t req_len;              /* length of queued TPM request */
> > +   size_t resp_len;             /* length of queued TPM response */
> > +   u8 buffer[TPM_BUFSIZE];      /* request/response buffer */
> 
> I'd use alloc_page() with GFP_USERHIGH in order to be a better citizen
> in 32-bit environments. You can kmap() it when you need it.
> 
> > +};
> > +
> > +
> > +static void vtpm_delete_device_pair(struct vtpm_dev *vtpm_dev);
> > +
> > +/*
> > + * Functions related to 'server side'
> > + */
> > +
> > +/**
> > + * vtpm_fops_read - Read TPM commands on 'server side'
> > + *
> > + * Return value:
> > + *   Number of bytes read or negative error code
> > + */
> > +static ssize_t vtpm_fops_read(struct file *filp, char __user *buf,
> > +               size_t count, loff_t *off)
> > +{
> > +   struct vtpm_dev *vtpm_dev = filp->private_data;
> > +   size_t len;
> > +   int sig, rc;
> > +
> > +   sig = wait_event_interruptible(vtpm_dev->wq, vtpm_dev->req_len != 
0);
> > +   if (sig)
> > +      return -EINTR;
> > +
> > +   spin_lock(&vtpm_dev->buf_lock);
> > +
> > +   len = vtpm_dev->req_len;
> > +
> > +   if (count < len) {
> > +      spin_unlock(&vtpm_dev->buf_lock);
> > +      pr_debug("Invalid size in recv: count=%zd, req_len=%zd\n",
> > +          count, len);
> > +      return -EIO;
> > +   }
> > +
> > +   rc = copy_to_user(buf, vtpm_dev->buffer, len);
> > +   memset(vtpm_dev->buffer, 0, len);
> > +   vtpm_dev->req_len = 0;
> 
> I saw already Jason's comment but maybe you could just use a mutex
> instead of a spinlock?

I'll let Jason respond to it whether it's the difference between spinlock 
and mutex or just no locking at all. If no locking, I'd be inclined to 
work with two buffers, one for requests and one for responses.



> > +
> > +/*
> > + * Called when core TPM driver reads TPM responses from 'server side'
> > + *
> > + * Return value:
> > + *      Number of TPM response bytes read, negative error value 
otherwise
> > + */
> > +static int vtpm_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t 
count)
> > +{
> > +   struct vtpm_dev *vtpm_dev = chip->vendor.priv;
> > +   int sig;
> > +   size_t len;
> > +
> > +   if (!vtpm_dev)
> > +      return -EIO;
> > +
> > +   /* wait for response or responder gone */
> > +   sig = wait_event_interruptible(vtpm_dev->wq,
> > +      (vtpm_dev->resp_len != 0
> > +      || !test_bit(STATE_OPENED_BIT, &vtpm_dev->state)));
> > +
> > +   if (sig)
> > +      return -EINTR;


With us not operating this driver in interrupt mode, we don't need the 
wait_event_interruptible if we make another change further below:


> > +
> > +static u8 vtpm_tpm_op_status(struct tpm_chip *chip)
> > +{
> > +   return 0;
> > +}


I modified this to 

static u8 vtpm_tpm_op_status(struct tpm_chip *chip)
{
        return (chip->resp_len) ? VTPM_REQ_COMPLETE_FLAG : 0;
}


> > +
> > +static bool vtpm_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
> > +{
> > +   return (status == 0);
> > +}

This will have to remain like this.

> > +
> > +static const struct tpm_class_ops vtpm_tpm_ops = {
> > +   .recv = vtpm_tpm_op_recv,
> > +   .send = vtpm_tpm_op_send,
> > +   .cancel = vtpm_tpm_op_cancel,
> > +   .status = vtpm_tpm_op_status,
> > +   .req_complete_mask = 0,
> > +   .req_complete_val = 0,

        .req_complete_mask = VTPM_REQ_COMPLETE_FLAG,
        .req_complete_val = VTPM_REQ_COMPLETE_FLAG,

with

#define VTPM_REQ_COMPLETE_FLAG  BIT(0)


Agreed?

   Stefan



[-- Attachment #1.2: Type: text/html, Size: 9332 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel