Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-26 Thread PrasannaKumar Muralidharan 
On 26 October 2017 at 00:41, Jarkko Sakkinen
 wrote:
> On Wed, Oct 25, 2017 at 08:21:16PM +0530, PrasannaKumar Muralidharan wrote:
>> >> > 2. Moving struct tpm_rng to the TPM client is architecturally
>> >> >uacceptable.
>> >>
>> >> As there was no response to the patch there is no way to know whether
>> >> it is acceptable or not.
>> >
>> > I like the idea of removing the tpm rng driver as discussed in other
>> > emails in this thread.
>>
>> Thank you.
>
> No, thank you.
>
> I didn't first understand the big idea and only looked at the code
> change per se. I apologize for that.

No need for that. I missed mentioning the reason for the patch and it
is not obvious from code change. Its my fault.

> The problem that you went to solve was real and it led to a properly
> implemented solution. You were not late from the party. Jason's code
> change is derivative work of your code change. That's why his code
> change has also your signed-off-by.
>
> Thanks for doing awesome work :-)

Its really nice to hear such words :-) :-D.

>
> /Jarkko

Thanks and regards,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-25 Thread Jarkko Sakkinen 
On Wed, Oct 25, 2017 at 08:21:16PM +0530, PrasannaKumar Muralidharan wrote:
> >> > 2. Moving struct tpm_rng to the TPM client is architecturally
> >> >uacceptable.
> >>
> >> As there was no response to the patch there is no way to know whether
> >> it is acceptable or not.
> >
> > I like the idea of removing the tpm rng driver as discussed in other
> > emails in this thread.
> 
> Thank you.

No, thank you.

I didn't first understand the big idea and only looked at the code
change per se. I apologize for that.

The problem that you went to solve was real and it led to a properly
implemented solution. You were not late from the party. Jason's code
change is derivative work of your code change. That's why his code
change has also your signed-off-by.

Thanks for doing awesome work :-)

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-25 Thread PrasannaKumar Muralidharan 
Hi Jarkko,

On 24 October 2017 at 23:52, Jarkko Sakkinen
 wrote:
> On Tue, Oct 24, 2017 at 10:05:20PM +0530, PrasannaKumar Muralidharan wrote:
>> > 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
>> >no other users.
>>
>> Completely agree that there is no in kernel users yet.
>
> And should never be. It's a bogus parameter that makes no sense.

I understood that after seeing latest patch that uses struct tpm_chip.
Sorry for the noise.

>> > 2. Moving struct tpm_rng to the TPM client is architecturally
>> >uacceptable.
>>
>> As there was no response to the patch there is no way to know whether
>> it is acceptable or not.
>
> I like the idea of removing the tpm rng driver as discussed in other
> emails in this thread.

Thank you.

>> > 3. Using zero deos not give you any better guarantees on anything than
>> >just using TPM_ANY_NUM.
>>
>> Chip id is used, not zero.
>
> Sorry I misread the patch first time. Anyway it's not any kind of ID to
> be trusted.

Okay.

>> > Why this patch is not CC'd to linux-integrity? It modifies the TPM
>> > driver. And in the worst way.
>>
>> TPM list is moderated and the moderator has not approved it yet.
>> get_maintainer script did not say about linux-integrity mailing list.
>>
>> It could be doing things in worst way but it is not known until some
>> one says. If no one tells it is the case I don't think it is possible
>> to fix. Which is what happened.
>
> Understood. We've moved to linux-integr...@vger.kernel.org. MAINTAINERS
> update is in the queue for the next kernel release.

Sorry I never knew this.

>> > Implementing the ideas that Jason explained is the senseful way to
>> > get stable access. modules.dep makes sure that the modules are loaded
>> > in the correct order.
>>
>> If that is sensible then it is the way to go.
>>
>> There must be a reason to believe what is sensible and what is not.
>> Looks like this RFC has helped in judging that.
>>
>> Regards,
>> PrasannaKumar
>
> Would you be interested to work on patch set that would remove the
> existing tpm rng driver and make the TPM driver the customer? It's not
> that far away from the work you've been doing already.
>
> /Jarkko

I am late to the party. Jason has sent a patch doing that by the time
I read this email.

Thanks and regards,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Peter Huewe 


Am 24. Oktober 2017 20:15:12 MESZ schrieb Jarkko Sakkinen 
:
>On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
>> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
>>  wrote:
>> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> Hi Jason,
>> >>
>> >> On 24 October 2017 at 21:25, Jason Gunthorpe
>> >>  wrote:
>> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> >
>> >> >> Please check the RFC [1]. It does use chip id. The rfc has
>issues and
>> >> >> has to be fixed but still there could be users of the API.
>> >> >>
>> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >> >
>> >> > That patch isn't safe at all. You need to store a kref to th
>chip in
>> >> > the hwrng, not parse a string.
>> >>
>> >> The drivers/char/hw_random/tpm-rng.c module does not store the
>chip
>> >> reference so I guess the usage is safe.
>> >
>> > It is using the default TPM, it is always safe to use the default
>tpm.
>> 
>> tpm-rng is abomination that should be kicked out as soon as possible.
>> It wrecks havoc with the power management (TPM chip drivers may go
>> into suspend state, but tpm_rng does not do any power management and
>> happily forwards requests to suspended hardware) and may be available
>> when there is no TPM at all yet (the drivers have not been probed
>yet,
>> or have gotten a deferral, etc).
>> 
>> TPM core should register HWRNGs when chips are ready.
>> 
>> Thanks.
>> 
>> -- 
>> Dmitry
>
>I'm fine to review a two patch set where:
>
>1. Patch 1 removes the existing TPM rng driver
>2. Patch 2 makes the TPM driver as rng producer

Yes, but tpm must be kept a hwrng source.
This is imho an important use case.

>
>Unrelate to patch that I'm proposing now but this sounds sensible.
>
>/Jarkko

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jarkko Sakkinen 
On Tue, Oct 24, 2017 at 10:05:20PM +0530, PrasannaKumar Muralidharan wrote:
> > 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
> >no other users.
> 
> Completely agree that there is no in kernel users yet.

And should never be. It's a bogus parameter that makes no sense.

> > 2. Moving struct tpm_rng to the TPM client is architecturally
> >uacceptable.
> 
> As there was no response to the patch there is no way to know whether
> it is acceptable or not.

I like the idea of removing the tpm rng driver as discussed in other
emails in this thread.

> > 3. Using zero deos not give you any better guarantees on anything than
> >just using TPM_ANY_NUM.
> 
> Chip id is used, not zero.

Sorry I misread the patch first time. Anyway it's not any kind of ID to
be trusted.

> > Why this patch is not CC'd to linux-integrity? It modifies the TPM
> > driver. And in the worst way.
> 
> TPM list is moderated and the moderator has not approved it yet.
> get_maintainer script did not say about linux-integrity mailing list.
> 
> It could be doing things in worst way but it is not known until some
> one says. If no one tells it is the case I don't think it is possible
> to fix. Which is what happened.

Understood. We've moved to linux-integr...@vger.kernel.org. MAINTAINERS
update is in the queue for the next kernel release.

> > Implementing the ideas that Jason explained is the senseful way to
> > get stable access. modules.dep makes sure that the modules are loaded
> > in the correct order.
> 
> If that is sensible then it is the way to go.
> 
> There must be a reason to believe what is sensible and what is not.
> Looks like this RFC has helped in judging that.
> 
> Regards,
> PrasannaKumar

Would you be interested to work on patch set that would remove the
existing tpm rng driver and make the TPM driver the customer? It's not
that far away from the work you've been doing already.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jarkko Sakkinen 
On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
>  wrote:
> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
> >> Hi Jason,
> >>
> >> On 24 October 2017 at 21:25, Jason Gunthorpe
> >>  wrote:
> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan 
> >> > wrote:
> >> >
> >> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
> >> >> has to be fixed but still there could be users of the API.
> >> >>
> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
> >> >
> >> > That patch isn't safe at all. You need to store a kref to th chip in
> >> > the hwrng, not parse a string.
> >>
> >> The drivers/char/hw_random/tpm-rng.c module does not store the chip
> >> reference so I guess the usage is safe.
> >
> > It is using the default TPM, it is always safe to use the default tpm.
> 
> tpm-rng is abomination that should be kicked out as soon as possible.
> It wrecks havoc with the power management (TPM chip drivers may go
> into suspend state, but tpm_rng does not do any power management and
> happily forwards requests to suspended hardware) and may be available
> when there is no TPM at all yet (the drivers have not been probed yet,
> or have gotten a deferral, etc).
> 
> TPM core should register HWRNGs when chips are ready.
> 
> Thanks.
> 
> -- 
> Dmitry

I'm fine to review a two patch set where:

1. Patch 1 removes the existing TPM rng driver
2. Patch 2 makes the TPM driver as rng producer

Unrelate to patch that I'm proposing now but this sounds sensible.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Dmitry Torokhov 
On Tue, Oct 24, 2017 at 11:37:57AM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
> > tpm-rng is abomination that should be kicked out as soon as possible.
> > It wrecks havoc with the power management (TPM chip drivers may go
> > into suspend state, but tpm_rng does not do any power management and
> > happily forwards requests to suspended hardware) and may be available
> > when there is no TPM at all yet (the drivers have not been probed yet,
> > or have gotten a deferral, etc).
> 
> Makes sense
> 
> > TPM core should register HWRNGs when chips are ready.
> 
> The main thing I've wanted from the TPM RNG is
> 'add_early_randomness'..

I see... However you can't add any randomness if hardware is not quite
there yet.

> 
> We can certainly provide a TPM interface to hwrng, it seems
> reasonable.
> 
> Excep that we already have a user api in /dev/tpm to access the
> tpm RNG, is the duplication a problem?

If we already have userspace API we have to maintain this, even if it is
duplicate, there is no way around it. I'd expect most of the users will
use unified random API, while some TPM-oriented users may still go via
/dev/tpm to use the same API as all other their requests.

Thanks.

-- 
Dmitry

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread PrasannaKumar Muralidharan 
Hi Jason,

On 24 October 2017 at 23:16, Jason Gunthorpe
 wrote:
> On Tue, Oct 24, 2017 at 09:44:30PM +0530, PrasannaKumar Muralidharan wrote:
>
>> I am wondering why it is wrong. Isn't the chip id valid till it is
>> unregistered? If so the rfc is correct. Please explain, may be I am
>> missing something.
>
> The lifetime is a bit complicated, but the general rule in the kernel
> for things like this it to use pointers, not ids, and certainly not
> string ids.
>
> For that patch it could just use container_of to get the chip..
>
> Jason

hwrng requires a unique name for every device. In that patch
"tpm-rng-" is used. chip_num is nothing but dev->dev_num.
This way more than 1 tpm chip can be used as rng provider.
tpm_get_random uses chip_num as its parameter. This is why
chip->dev_num was used.

Is that reasoning correct?

Please feel free to correct me if I am wrong.

Thanks,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jason Gunthorpe 
On Tue, Oct 24, 2017 at 09:44:30PM +0530, PrasannaKumar Muralidharan wrote:

> I am wondering why it is wrong. Isn't the chip id valid till it is
> unregistered? If so the rfc is correct. Please explain, may be I am
> missing something.

The lifetime is a bit complicated, but the general rule in the kernel
for things like this it to use pointers, not ids, and certainly not
string ids.

For that patch it could just use container_of to get the chip..

Jason

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread PrasannaKumar Muralidharan 
On 24 October 2017 at 23:07, Jason Gunthorpe
 wrote:
> On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
>> tpm-rng is abomination that should be kicked out as soon as possible.
>> It wrecks havoc with the power management (TPM chip drivers may go
>> into suspend state, but tpm_rng does not do any power management and
>> happily forwards requests to suspended hardware) and may be available
>> when there is no TPM at all yet (the drivers have not been probed yet,
>> or have gotten a deferral, etc).
>
> Makes sense
>
>> TPM core should register HWRNGs when chips are ready.
>
> The main thing I've wanted from the TPM RNG is
> 'add_early_randomness'..
>
> We can certainly provide a TPM interface to hwrng, it seems
> reasonable.
>
> Excep that we already have a user api in /dev/tpm to access the
> tpm RNG, is the duplication a problem?

I tried to do that via the rfc we discussed previously. It may not be
the right way but I wanted to start the discussion via the rfc.

Thanks,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jason Gunthorpe 
On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
> tpm-rng is abomination that should be kicked out as soon as possible.
> It wrecks havoc with the power management (TPM chip drivers may go
> into suspend state, but tpm_rng does not do any power management and
> happily forwards requests to suspended hardware) and may be available
> when there is no TPM at all yet (the drivers have not been probed yet,
> or have gotten a deferral, etc).

Makes sense

> TPM core should register HWRNGs when chips are ready.

The main thing I've wanted from the TPM RNG is
'add_early_randomness'..

We can certainly provide a TPM interface to hwrng, it seems
reasonable.

Excep that we already have a user api in /dev/tpm to access the
tpm RNG, is the duplication a problem?

Jason

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Dmitry Torokhov 
On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
 wrote:
> On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 24 October 2017 at 21:25, Jason Gunthorpe
>>  wrote:
>> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> >> has to be fixed but still there could be users of the API.
>> >>
>> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >
>> > That patch isn't safe at all. You need to store a kref to th chip in
>> > the hwrng, not parse a string.
>>
>> The drivers/char/hw_random/tpm-rng.c module does not store the chip
>> reference so I guess the usage is safe.
>
> It is using the default TPM, it is always safe to use the default tpm.

tpm-rng is abomination that should be kicked out as soon as possible.
It wrecks havoc with the power management (TPM chip drivers may go
into suspend state, but tpm_rng does not do any power management and
happily forwards requests to suspended hardware) and may be available
when there is no TPM at all yet (the drivers have not been probed yet,
or have gotten a deferral, etc).

TPM core should register HWRNGs when chips are ready.

Thanks.

-- 
Dmitry

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread PrasannaKumar Muralidharan 
On 24 October 2017 at 21:53, Jarkko Sakkinen
 wrote:
> On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> On 24 October 2017 at 21:14, Jarkko Sakkinen
>>  wrote:
>> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>> >>
>> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> >> > >  {
>> >> >
>> >> >
>> >> > I think every kernel internal TPM driver API should be called with the
>> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA 
>> >> > where we
>> >> > want to provide the flexibility of passing a dedicated vTPM to each
>> >> > namespace and IMA would use the chip as a parameter to all of these
>> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> >> > perspective this patch goes into the wrong direction.
>> >>
>> >> Yes, we should ultimately try and get to there.. Someday the
>> >> tpm_chip_find_get() will need to become namespace aware.
>> >>
>> >> But this patch is along the right path, eliminating the chip_num is
>> >> the right thing to do..
>> >>
>> >> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> >> > >+  tpm2 = tpm_is_tpm2();
>> >> > >   if (tpm2 < 0)
>> >> > >   return tpm2;
>> >> > >
>> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> >> > >   switch (key_cmd) {
>> >> > >   case Opt_load:
>> >> > >   if (tpm2)
>> >> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, 
>> >> > >options);
>> >> > >+  ret = tpm_unseal_trusted(payload, options);
>> >>
>> >> Sequences like this are sketchy.
>> >>
>> >> It should be
>> >>
>> >> struct tpm_chip *tpm;
>> >>
>> >> tpm = tpm_chip_find_get()
>> >> tpm2 = tpm_is_tpm2(tpm);
>> >>
>> >> [..]
>> >>
>> >> if (tpm2)
>> >>  ret = tpm_unseal_trusted(tpm, payload, options);
>> >>
>> >> [..]
>> >>
>> >> tpm_put_chip(tpm);
>> >>
>> >> As hot plug could alter the 'tpm' between the two tpm calls.
>> >>
>> >> Jason
>> >
>> > This patch just removes bunch of dead code. It does not change existing
>> > semantics. What you are saying should be done after the dead code has
>> > been removed. This commit is first step to that direction.
>> >
>> > /Jarkko
>>
>> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> has to be fixed but still there could be users of the API.
>>
>> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>>
>> Regards,
>> PrasannaKumar
>
> 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
>no other users.

Completely agree that there is no in kernel users yet.

> 2. Moving struct tpm_rng to the TPM client is architecturally
>uacceptable.

As there was no response to the patch there is no way to know whether
it is acceptable or not.

> 3. Using zero deos not give you any better guarantees on anything than
>just using TPM_ANY_NUM.

Chip id is used, not zero.

> Why this patch is not CC'd to linux-integrity? It modifies the TPM
> driver. And in the worst way.

TPM list is moderated and the moderator has not approved it yet.
get_maintainer script did not say about linux-integrity mailing list.

It could be doing things in worst way but it is not known until some
one says. If no one tells it is the case I don't think it is possible
to fix. Which is what happened.

> Implementing the ideas that Jason explained is the senseful way to
> get stable access. modules.dep makes sure that the modules are loaded
> in the correct order.

If that is sensible then it is the way to go.

There must be a reason to believe what is sensible and what is not.
Looks like this RFC has helped in judging that.

Regards,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jarkko Sakkinen 
On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
> On 24 October 2017 at 21:14, Jarkko Sakkinen
>  wrote:
> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> >>
> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> >> > >  {
> >> >
> >> >
> >> > I think every kernel internal TPM driver API should be called with the
> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA 
> >> > where we
> >> > want to provide the flexibility of passing a dedicated vTPM to each
> >> > namespace and IMA would use the chip as a parameter to all of these
> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
> >> > perspective this patch goes into the wrong direction.
> >>
> >> Yes, we should ultimately try and get to there.. Someday the
> >> tpm_chip_find_get() will need to become namespace aware.
> >>
> >> But this patch is along the right path, eliminating the chip_num is
> >> the right thing to do..
> >>
> >> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> >> > >+  tpm2 = tpm_is_tpm2();
> >> > >   if (tpm2 < 0)
> >> > >   return tpm2;
> >> > >
> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> >> > >   switch (key_cmd) {
> >> > >   case Opt_load:
> >> > >   if (tpm2)
> >> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, 
> >> > >options);
> >> > >+  ret = tpm_unseal_trusted(payload, options);
> >>
> >> Sequences like this are sketchy.
> >>
> >> It should be
> >>
> >> struct tpm_chip *tpm;
> >>
> >> tpm = tpm_chip_find_get()
> >> tpm2 = tpm_is_tpm2(tpm);
> >>
> >> [..]
> >>
> >> if (tpm2)
> >>  ret = tpm_unseal_trusted(tpm, payload, options);
> >>
> >> [..]
> >>
> >> tpm_put_chip(tpm);
> >>
> >> As hot plug could alter the 'tpm' between the two tpm calls.
> >>
> >> Jason
> >
> > This patch just removes bunch of dead code. It does not change existing
> > semantics. What you are saying should be done after the dead code has
> > been removed. This commit is first step to that direction.
> >
> > /Jarkko
> 
> Please check the RFC [1]. It does use chip id. The rfc has issues and
> has to be fixed but still there could be users of the API.
> 
> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
> 
> Regards,
> PrasannaKumar

1. Every user in the kernel is using TPM_ANY_NUM, which means there are
   no other users.
2. Moving struct tpm_rng to the TPM client is architecturally
   uacceptable.
3. Using zero deos not give you any better guarantees on anything than
   just using TPM_ANY_NUM.

Why this patch is not CC'd to linux-integrity? It modifies the TPM
driver. And in the worst way.

Implementing the ideas that Jason explained is the senseful way to
get stable access. modules.dep makes sure that the modules are loaded
in the correct order.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread PrasannaKumar Muralidharan 
On 24 October 2017 at 21:41, Jason Gunthorpe
 wrote:
> On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 24 October 2017 at 21:25, Jason Gunthorpe
>>  wrote:
>> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> >> has to be fixed but still there could be users of the API.
>> >>
>> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >
>> > That patch isn't safe at all. You need to store a kref to th chip in
>> > the hwrng, not parse a string.
>>
>> The drivers/char/hw_random/tpm-rng.c module does not store the chip
>> reference so I guess the usage is safe.
>
> It is using the default TPM, it is always safe to use the default tpm.
>
>> The RFC is just a sample use case of the API.
>
> Well, a wrong example not to be emulated, and I think, further shows
> how Jarkko's direction is the right one.

I am wondering why it is wrong. Isn't the chip id valid till it is
unregistered? If so the rfc is correct. Please explain, may be I am
missing something.

Thanks,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jason Gunthorpe 
On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
> Hi Jason,
> 
> On 24 October 2017 at 21:25, Jason Gunthorpe
>  wrote:
> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
> >
> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
> >> has to be fixed but still there could be users of the API.
> >>
> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
> >
> > That patch isn't safe at all. You need to store a kref to th chip in
> > the hwrng, not parse a string.
> 
> The drivers/char/hw_random/tpm-rng.c module does not store the chip
> reference so I guess the usage is safe.

It is using the default TPM, it is always safe to use the default tpm.

> The RFC is just a sample use case of the API.

Well, a wrong example not to be emulated, and I think, further shows
how Jarkko's direction is the right one.

Jason

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread PrasannaKumar Muralidharan 
Hi Jason,

On 24 October 2017 at 21:25, Jason Gunthorpe
 wrote:
> On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>
>> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> has to be fixed but still there could be users of the API.
>>
>> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>
> That patch isn't safe at all. You need to store a kref to th chip in
> the hwrng, not parse a string.

The drivers/char/hw_random/tpm-rng.c module does not store the chip
reference so I guess the usage is safe. The RFC is just a sample use
case of the API.

Regards,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jason Gunthorpe 
On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:

> Please check the RFC [1]. It does use chip id. The rfc has issues and
> has to be fixed but still there could be users of the API.
> 
> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html

That patch isn't safe at all. You need to store a kref to th chip in
the hwrng, not parse a string.

Jason

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread PrasannaKumar Muralidharan 
On 24 October 2017 at 21:14, Jarkko Sakkinen
 wrote:
> On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>>
>> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> > >  {
>> >
>> >
>> > I think every kernel internal TPM driver API should be called with the
>> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where 
>> > we
>> > want to provide the flexibility of passing a dedicated vTPM to each
>> > namespace and IMA would use the chip as a parameter to all of these
>> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> > perspective this patch goes into the wrong direction.
>>
>> Yes, we should ultimately try and get to there.. Someday the
>> tpm_chip_find_get() will need to become namespace aware.
>>
>> But this patch is along the right path, eliminating the chip_num is
>> the right thing to do..
>>
>> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> > >+  tpm2 = tpm_is_tpm2();
>> > >   if (tpm2 < 0)
>> > >   return tpm2;
>> > >
>> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> > >   switch (key_cmd) {
>> > >   case Opt_load:
>> > >   if (tpm2)
>> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, 
>> > >options);
>> > >+  ret = tpm_unseal_trusted(payload, options);
>>
>> Sequences like this are sketchy.
>>
>> It should be
>>
>> struct tpm_chip *tpm;
>>
>> tpm = tpm_chip_find_get()
>> tpm2 = tpm_is_tpm2(tpm);
>>
>> [..]
>>
>> if (tpm2)
>>  ret = tpm_unseal_trusted(tpm, payload, options);
>>
>> [..]
>>
>> tpm_put_chip(tpm);
>>
>> As hot plug could alter the 'tpm' between the two tpm calls.
>>
>> Jason
>
> This patch just removes bunch of dead code. It does not change existing
> semantics. What you are saying should be done after the dead code has
> been removed. This commit is first step to that direction.
>
> /Jarkko

Please check the RFC [1]. It does use chip id. The rfc has issues and
has to be fixed but still there could be users of the API.

1. https://www.spinics.net/lists/linux-crypto/msg28282.html

Regards,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jarkko Sakkinen 
On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> 
> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> > >  {
> > 
> > 
> > I think every kernel internal TPM driver API should be called with the
> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> > want to provide the flexibility of passing a dedicated vTPM to each
> > namespace and IMA would use the chip as a parameter to all of these
> > functions to talk to the right tpm_vtpm_proxy instance. From that
> > perspective this patch goes into the wrong direction.
> 
> Yes, we should ultimately try and get to there.. Someday the
> tpm_chip_find_get() will need to become namespace aware.
> 
> But this patch is along the right path, eliminating the chip_num is
> the right thing to do..
> 
> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> > >+  tpm2 = tpm_is_tpm2();
> > >   if (tpm2 < 0)
> > >   return tpm2;
> > >
> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> > >   switch (key_cmd) {
> > >   case Opt_load:
> > >   if (tpm2)
> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> > >+  ret = tpm_unseal_trusted(payload, options);
> 
> Sequences like this are sketchy.
> 
> It should be
> 
> struct tpm_chip *tpm;
> 
> tpm = tpm_chip_find_get()
> tpm2 = tpm_is_tpm2(tpm);
> 
> [..]
> 
> if (tpm2)
>  ret = tpm_unseal_trusted(tpm, payload, options);
> 
> [..]
> 
> tpm_put_chip(tpm);
> 
> As hot plug could alter the 'tpm' between the two tpm calls.
> 
> Jason

This patch just removes bunch of dead code. It does not change existing
semantics. What you are saying should be done after the dead code has
been removed. This commit is first step to that direction.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-24 Thread Jarkko Sakkinen 
On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
> I think every kernel internal TPM driver API should be called with the
> tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> want to provide the flexibility of passing a dedicated vTPM to each
> namespace and IMA would use the chip as a parameter to all of these
> functions to talk to the right tpm_vtpm_proxy instance. From that
> perspective this patch goes into the wrong direction.
> 
>Stefan

The goal of this patch is to kernel code that never gets executed. It
removes a load of completely dead code. It is the only thing that this
commit does. Why do you think this is "going into wrong direction" if it
only removes dead code and refines the documentation up to date?

After the dead code has been removed it makes sense to propose a better
mechanism. Maybe the one that you are speaking about. But you need to
remove the cruft first.

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-23 Thread Jason Gunthorpe 
On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:

> >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
> >  {
> 
> 
> I think every kernel internal TPM driver API should be called with the
> tpm_chip as a parameter. This is in foresight of namespacing of IMA where we
> want to provide the flexibility of passing a dedicated vTPM to each
> namespace and IMA would use the chip as a parameter to all of these
> functions to talk to the right tpm_vtpm_proxy instance. From that
> perspective this patch goes into the wrong direction.

Yes, we should ultimately try and get to there.. Someday the
tpm_chip_find_get() will need to become namespace aware.

But this patch is along the right path, eliminating the chip_num is
the right thing to do..

> >-tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> >+tpm2 = tpm_is_tpm2();
> > if (tpm2 < 0)
> > return tpm2;
> >
> >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> > switch (key_cmd) {
> > case Opt_load:
> > if (tpm2)
> >-ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
> >+ret = tpm_unseal_trusted(payload, options);

Sequences like this are sketchy.

It should be

struct tpm_chip *tpm;

tpm = tpm_chip_find_get()
tpm2 = tpm_is_tpm2(tpm);

[..]

if (tpm2)
 ret = tpm_unseal_trusted(tpm, payload, options);

[..]

tpm_put_chip(tpm);

As hot plug could alter the 'tpm' between the two tpm calls.

Jason

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

2017-10-23 Thread Stefan Berger 

On 10/23/2017 08:38 AM, Jarkko Sakkinen wrote:

The reasoning is simple and obvious. Since every call site passes the
value TPM_ANY_NUM (0x) the parameter does not have right to exist.
Refined the documentation of the corresponding functions.

Signed-off-by: Jarkko Sakkinen 
---
  drivers/char/hw_random/tpm-rng.c|  2 +-
  drivers/char/tpm/tpm-chip.c | 38 
  drivers/char/tpm/tpm-interface.c| 87 ++---
  drivers/char/tpm/tpm.h  |  2 +-
  include/linux/tpm.h | 43 --
  security/integrity/ima/ima_crypto.c |  2 +-
  security/integrity/ima/ima_init.c   |  2 +-
  security/integrity/ima/ima_queue.c  |  2 +-
  security/keys/trusted.c | 35 ---
  9 files changed, 101 insertions(+), 112 deletions(-)

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d448266f07..8823efcddab8 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -25,7 +25,7 @@

  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
  {
-   return tpm_get_random(TPM_ANY_NUM, data, max);
+   return tpm_get_random(data, max);
  }

  static struct hwrng tpm_rng = {
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a114e8f7fb90..ec35643b 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -81,34 +81,32 @@ void tpm_put_ops(struct tpm_chip *chip)
  EXPORT_SYMBOL_GPL(tpm_put_ops);

  /**
- * tpm_chip_find_get() - return tpm_chip for a given chip number
- * @chip_num: id to find
+ * tpm_chip_find_get() - reserved the first available TPM chip
   *
- * The return'd chip has been tpm_try_get_ops'd and must be released via
- * tpm_put_ops
+ * Finds the first available TPM chip and reserves its class device and
+ * operations.
+ *
+ * Return: a reserved  tpm_chip instance
   */
-struct tpm_chip *tpm_chip_find_get(int chip_num)
+struct tpm_chip *tpm_chip_find_get(void)
  {
-   struct tpm_chip *chip, *res = NULL;
+   struct tpm_chip *chip;
+   struct tpm_chip *res = NULL;
int chip_prev;
+   int chip_num;

mutex_lock(_lock);

-   if (chip_num == TPM_ANY_NUM) {
-   chip_num = 0;
-   do {
-   chip_prev = chip_num;
-   chip = idr_get_next(_nums_idr, _num);
-   if (chip && !tpm_try_get_ops(chip)) {
-   res = chip;
-   break;
-   }
-   } while (chip_prev != chip_num);
-   } else {
-   chip = idr_find(_nums_idr, chip_num);
-   if (chip && !tpm_try_get_ops(chip))
+   chip_num = 0;
+
+   do {
+   chip_prev = chip_num;
+   chip = idr_get_next(_nums_idr, _num);
+   if (chip && !tpm_try_get_ops(chip)) {
res = chip;
-   }
+   break;
+   }
+   } while (chip_prev != chip_num);

mutex_unlock(_lock);



Here you are keeping the loop, which I think is good and I would like 
you to keep it...




diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d8e2e5bca903..b3907d3556ce 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -802,18 +802,19 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, 
u8 *res_buf)
  }

  /**
- * tpm_is_tpm2 - is the chip a TPM2 chip?
- * @chip_num:  tpm idx # or ANY
+ * tpm_is_tpm2 - do we a have a TPM2 chip?
   *
- * Returns < 0 on error, and 1 or 0 on success depending whether the chip
- * is a TPM2 chip.
+ * Return:
+ * 1 if we have a TPM2 chip.
+ * 0 if we don't have a TPM2 chip.
+ * A negative number for system errors (errno).
   */
-int tpm_is_tpm2(u32 chip_num)
+int tpm_is_tpm2(void)
  {
struct tpm_chip *chip;
int rc;

-   chip = tpm_chip_find_get(chip_num);
+   chip = tpm_chip_find_get();
if (chip == NULL)
return -ENODEV;

@@ -826,22 +827,18 @@ int tpm_is_tpm2(u32 chip_num)
  EXPORT_SYMBOL_GPL(tpm_is_tpm2);

  /**
- * tpm_pcr_read - read a pcr value
- * @chip_num:  tpm idx # or ANY
- * @pcr_idx:   pcr idx to retrieve
- * @res_buf:   TPM_PCR value
- * size of res_buf is 20 bytes (or NULL if you don't care)
+ * tpm_pcr_read - read a PCR value from SHA1 bank
+ * @pcr_idx:   the PCR to be retrieved
+ * @res_buf:   the value of the PCR
   *
- * The TPM driver should be built-in, but for whatever reason it
- * isn't, protect against the chip disappearing, by incrementing
- * the module usage count.
+ * Return: same as with tpm_transmit_cmd()
   */
-int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(int pcr_idx, u8 *res_buf)
  {
struct tpm_chip *chip;
int rc;

-   chip = tpm_chip_find_get(chip_num);