Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1157513 > unrolled thread

Re: [PATCH] tpm: introduce struct tpm_buf

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] tpm: introduce struct tpm_buf Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-06-03 14:40 +0200
    Re: [PATCH] tpm: introduce struct tpm_buf Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-06-03 18:20 +0200
      Re: [PATCH] tpm: introduce struct tpm_buf Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-06-03 18:50 +0200
        Re: [PATCH] tpm: introduce struct tpm_buf Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-06-03 19:10 +0200
      Re: [PATCH] tpm: introduce struct tpm_buf Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-06-03 18:50 +0200

#1157513 — Re: [PATCH] tpm: introduce struct tpm_buf

On Tue, Jun 02, 2015 at 12:13:15PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 02, 2015 at 04:04:22PM +0300, Jarkko Sakkinen wrote:
> > +/* A string buffer type for constructing TPM commands. This is based on the
> > + * code in security/keys/trusted.h.
> > + */
> > +
> > +#define TPM_BUF_SIZE 512
> > +
> > +struct tpm_buf {
> > +	u8 data[TPM_BUF_SIZE];
> This should be u32 or u64 to guarentee correct alignment for the
> casting.

Good catch. The functions where this might cause trouble are *_length()
and *_tag().

In other places misalignment should not cause any regressions since data
is not directly assigned to the buffer with pointer casting.

I would prefer to fix by changing *_length() and *_tag() to copy the
value to a local variable and return that. It's a fail safe way and here
the performance is not an issue.

> > +};
> > +
> > +static inline void tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +{
> > +	struct tpm_input_header *head;
> > +
> > +	head = (struct tpm_input_header *) buf->data;
> > +
> > +	head->tag = cpu_to_be16(tag);
> > +	head->length = cpu_to_be32(sizeof(*head));
> > +	head->ordinal = cpu_to_be32(ordinal);
> > +}
> > +
> > +static inline u32 tpm_buf_length(struct tpm_buf *buf)
> > +{
> > +	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
> > +
> > +	return be32_to_cpu(head->length);
> > +}
> > +
> > +static inline u16 tpm_buf_tag(struct tpm_buf *buf)
> > +{
> > +	return be16_to_cpu(*(__be16 *) &buf->data[0]);
> 
> be16_to_cpup ?

Thanks, I'll change this.

> Any thought on someday using this for tpm1 as well?

Yes, I think this could form the baseline so that the sealing code that
currently resides is security/keys/trusted.c could be eventually moved
to drivers/char/tpm.

I will be contributing API to include/linux/tpm.h that can be then
called by trusted keys. I will also contribute the implementation for
TPM 2.0 sealing but it is up to those who implemented TPM 1.x sealing
code to do the migration. I put all the enablers in place so that doing
that transition should be relatively painless.

I have PoC for TPM 2.0 trusted keys written in Python available here:

https://github.com/jsakkine/tpm2-scripts/blob/master/tpm2.py

(In kernel version the default hash algorithm should be probably SHA-256
as SHA-1 is not considered secure by NIST anymore)

Doing also all the TPM 1.x migration on top of this would make
oversubscribed.

> Jason

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1157811

On Wed, Jun 03, 2015 at 03:23:31PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 02, 2015 at 12:13:15PM -0600, Jason Gunthorpe wrote:
> > On Tue, Jun 02, 2015 at 04:04:22PM +0300, Jarkko Sakkinen wrote:
> > > +/* A string buffer type for constructing TPM commands. This is based on the
> > > + * code in security/keys/trusted.h.
> > > + */
> > > +
> > > +#define TPM_BUF_SIZE 512
> > > +
> > > +struct tpm_buf {
> > > +	u8 data[TPM_BUF_SIZE];
> > This should be u32 or u64 to guarentee correct alignment for the
> > casting.
> 
> Good catch. The functions where this might cause trouble are *_length()
> and *_tag().
> 
> In other places misalignment should not cause any regressions since data
> is not directly assigned to the buffer with pointer casting.
> 
> I would prefer to fix by changing *_length() and *_tag() to copy the
> value to a local variable and return that. It's a fail safe way and here
> the performance is not an issue.

I would change the type, that is very simple and will improve
performance of the memcpy as well.

It actually isn't a problem for the casts to tpm_input_header - any
structure marked __packed will cause the compiler to assume that the
entire structure is unaligned and code gen accordingly, so the cast
will always work, but on x86 it will be more efficient if the array
is aligned.

Ideally we could change to something like:
 __attribute__((packed,aligned(4))) 

Instead of __packed which will help the compiler minimize unaligned
load instructions..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1157836

On Wed, Jun 03, 2015 at 07:43:34PM +0300, Jarkko Sakkinen wrote:
> I realized basically the same what you said here when I refined the
> patch (read this email after sending v2) :) I decided that align to 8
> bytes.

Don't use the __attribute__((align)), use just a u64, the effect is
identical, and the latter doesn't rely on compiler specific features.

> I don't think the packed attribute is needed here because now I cast
> the beginning to tpm_input_header, which is a packed struct.

I ment the attributs could be changed on the tpm_input_header, the buf
should just be a u64 array..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1157882

On Wed, Jun 03, 2015 at 10:48:51AM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 03, 2015 at 07:43:34PM +0300, Jarkko Sakkinen wrote:
> > I realized basically the same what you said here when I refined the
> > patch (read this email after sending v2) :) I decided that align to 8
> > bytes.
> 
> Don't use the __attribute__((align)), use just a u64, the effect is
> identical, and the latter doesn't rely on compiler specific features.

Byte array is otherwise more convenient and this feature is widely used:

$ git grep "__attribute__((aligned" | wc -l
222

I don't see your point here.

> > I don't think the packed attribute is needed here because now I cast
> > the beginning to tpm_input_header, which is a packed struct.
> 
> I ment the attributs could be changed on the tpm_input_header, the buf
> should just be a u64 array..
> 
> Jason

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1157838

On Wed, Jun 03, 2015 at 10:11:28AM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 03, 2015 at 03:23:31PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Jun 02, 2015 at 12:13:15PM -0600, Jason Gunthorpe wrote:
> > > On Tue, Jun 02, 2015 at 04:04:22PM +0300, Jarkko Sakkinen wrote:
> > > > +/* A string buffer type for constructing TPM commands. This is based on the
> > > > + * code in security/keys/trusted.h.
> > > > + */
> > > > +
> > > > +#define TPM_BUF_SIZE 512
> > > > +
> > > > +struct tpm_buf {
> > > > +	u8 data[TPM_BUF_SIZE];
> > > This should be u32 or u64 to guarentee correct alignment for the
> > > casting.
> > 
> > Good catch. The functions where this might cause trouble are *_length()
> > and *_tag().
> > 
> > In other places misalignment should not cause any regressions since data
> > is not directly assigned to the buffer with pointer casting.
> > 
> > I would prefer to fix by changing *_length() and *_tag() to copy the
> > value to a local variable and return that. It's a fail safe way and here
> > the performance is not an issue.
> 
> I would change the type, that is very simple and will improve
> performance of the memcpy as well.
> 
> It actually isn't a problem for the casts to tpm_input_header - any
> structure marked __packed will cause the compiler to assume that the
> entire structure is unaligned and code gen accordingly, so the cast
> will always work, but on x86 it will be more efficient if the array
> is aligned.
> 
> Ideally we could change to something like:
>  __attribute__((packed,aligned(4))) 
> 
> Instead of __packed which will help the compiler minimize unaligned
> load instructions..

I realized basically the same what you said here when I refined the
patch (read this email after sending v2) :) I decided that align to 8
bytes.

I don't think the packed attribute is needed here because now I cast
the beginning to tpm_input_header, which is a packed struct.

> Jason

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web