| 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
| ||
|
Message-ID: <509AA501.1090701@jhuapl.edu>
Date: Wed, 07 Nov 2012 13:14:25 -0500
From: Matthew Fioravante <matthew.fioravante@...apl.edu>
To: Kent Yoder <key@...ux.vnet.ibm.com>
CC: "mail@...jiv.net" <mail@...jiv.net>,
"jeremy@...p.org" <jeremy@...p.org>,
"tpmdd-devel@...ts.sourceforge.net"
<tpmdd-devel@...ts.sourceforge.net>,
"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
"konrad.wilk@...cle.com" <konrad.wilk@...cle.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] add tpm_xenu.ko: Xen Virtual TPM frontend driver
On 11/07/2012 09:46 AM, Kent Yoder wrote:
> Hi Matthew,
>
> On Mon, Nov 05, 2012 at 10:09:57AM -0500, Matthew Fioravante wrote:
>> This patch ports the xen vtpm frontend driver for linux
>> from the linux-2.6.18-xen.hg tree to linux-stable.
>>
>> Signed-off-by: Matthew Fioravante <matthew.fioravante@...apl.edu>
>> ---
>> drivers/char/tpm/Kconfig | 9 +
>> drivers/char/tpm/Makefile | 2 +
>> drivers/char/tpm/tpm.h | 15 +
>> drivers/char/tpm/tpm_vtpm.c | 543 ++++++++++++++++++++++++++++++
>> drivers/char/tpm/tpm_vtpm.h | 55 +++
>> drivers/char/tpm/tpm_xen.c | 690 ++++++++++++++++++++++++++++++++++++++
>> include/xen/interface/io/tpmif.h | 77 +++++
>> 7 files changed, 1391 insertions(+)
>> create mode 100644 drivers/char/tpm/tpm_vtpm.c
>> create mode 100644 drivers/char/tpm/tpm_vtpm.h
>> create mode 100644 drivers/char/tpm/tpm_xen.c
>> create mode 100644 include/xen/interface/io/tpmif.h
>>
>> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
>> index 915875e..08c1010 100644
>> --- a/drivers/char/tpm/Kconfig
>> +++ b/drivers/char/tpm/Kconfig
>> @@ -81,4 +81,13 @@ config TCG_IBMVTPM
>> will be accessible from within Linux. To compile this driver
>> as a module, choose M here; the module will be called tpm_ibmvtpm.
>>
>> +config TCG_XEN
>> + tristate "XEN TPM Interface"
>> + depends on TCG_TPM && XEN
>> + ---help---
>> + If you want to make TPM support available to a Xen user domain,
>> + say Yes and it will be accessible from within Linux.
>> + To compile this driver as a module, choose M here; the module
>> + will be called tpm_xenu.
>> +
>> endif # TCG_TPM
>> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
>> index 5b3fc8b..16911c5 100644
>> --- a/drivers/char/tpm/Makefile
>> +++ b/drivers/char/tpm/Makefile
>> @@ -17,3 +17,5 @@ obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
>> obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
>> obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
>> obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
>> +obj-$(CONFIG_TCG_XEN) += tpm_xenu.o
>> +tpm_xenu-y = tpm_xen.o tpm_vtpm.o
> Let's match the naming convention of the other drivers if we can, so this
> would be something like tpm_xenvtpm.c. tpm_vtpm is too generic...
Makes sense, fixed.
>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 8ef7649..2e5a47a 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -130,6 +130,9 @@ struct tpm_chip {
>>
>> struct list_head list;
>> void (*release) (struct device *);
>> +#if CONFIG_XEN
>> + void *priv;
>> +#endif
> Can you use the chip->vendor.data pointer here instead? tpm_ibmvtpm is
> already using that as a priv pointer. I should probably change that name
> to make it more obvious what that's used for.
That makes more sense. I'm guessing your data pointer didn't exist
during the 2.6.18 kernel which is why they added their own priv pointer.
>
>> };
>>
>> #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
>> @@ -310,6 +313,18 @@ struct tpm_cmd_t {
>>
>> ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *);
>>
>> +#ifdef CONFIG_XEN
>> +static inline void *chip_get_private(const struct tpm_chip *chip)
>> +{
>> + return chip->priv;
>> +}
>> +
>> +static inline void chip_set_private(struct tpm_chip *chip, void *priv)
>> +{
>> + chip->priv = priv;
>> +}
>> +#endif
> Can you put these in tpm_vtpm.c please? One less #define. :-)
Agreed, I'd rather not have to modify your shared tpm.h interface at all.
>
> [cut]
>> +#ifndef __XEN_PUBLIC_IO_TPMIF_H__
>> +#define __XEN_PUBLIC_IO_TPMIF_H__
>> +
>> +#include "../grant_table.h"
>> +
>> +struct tpmif_tx_request {
>> + unsigned long addr; /* Machine address of packet. */
>> + grant_ref_t ref; /* grant table access reference */
>> + uint16_t unused;
>> + uint16_t size; /* Packet size in bytes. */
>> +};
>> +typedef struct tpmif_tx_request tpmif_tx_request_t;
> checkpatch warned on this new typedef - please run through checkpatch
> and fix up that stuff.
tpmif.h has a couple of typedefs which do trigger checkpatch warnings.
However it looks like the paradigm for xen is to have these
interface/io/<dev>if.h files and all of them have typedefs. I think in
this case the typedef should probably stay.
Konrad your thoughts here?
>
>> +
>> +/*
>> + * The TPMIF_TX_RING_SIZE defines the number of pages the
>> + * front-end and backend can exchange (= size of array).
>> + */
>> +typedef uint32_t TPMIF_RING_IDX;
>> +
>> +#define TPMIF_TX_RING_SIZE 1
>> +
>> +/* This structure must fit in a memory page. */
>> +
>> +struct tpmif_ring {
>> + struct tpmif_tx_request req;
>> +};
>> +typedef struct tpmif_ring tpmif_ring_t;
>> +
>> +struct tpmif_tx_interface {
>> + struct tpmif_ring ring[TPMIF_TX_RING_SIZE];
>> +};
>> +typedef struct tpmif_tx_interface tpmif_tx_interface_t;
>> +
>> +#endif
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-set-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> Please take this comment out, see Documentation/CodingStyle.
Fixed
>
> Thanks,
> Kent
>
>> --
>> 1.7.10.4
>>
>>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (1459 bytes)
Powered by blists - more mailing lists