2021-02-03 00:54:45

by Lino Sanfilippo

Subject: [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid

From: Lino Sanfilippo <[email protected]>

In tpm2_del_space() the sessions are flushed by means of the tpm_chip
operations. However the concerning operations pointer my already be NULL at
this time in case that the chip has been unregistered (see
tpm_chip_unregister() which calls tpm_del_char_device() which sets
chip->ops to NULL).
Before accessing chip->ops check if it is still valid. Skip flushing
the sessions in this case.

Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/char/tpm/tpm2-space.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 784b8b3..9a29a40 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -58,12 +58,17 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)

void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
{
- mutex_lock(&chip->tpm_mutex);
- if (!tpm_chip_start(chip)) {
- tpm2_flush_sessions(chip, space);
- tpm_chip_stop(chip);
+ down_read(&chip->ops_sem);
+ if (chip->ops) {
+ mutex_lock(&chip->tpm_mutex);
+ if (!tpm_chip_start(chip)) {
+ tpm2_flush_sessions(chip, space);
+ tpm_chip_stop(chip);
+ }
+ mutex_unlock(&chip->tpm_mutex);
}
- mutex_unlock(&chip->tpm_mutex);
+ up_read(&chip->ops_sem);
+
kfree(space->context_buf);
kfree(space->session_buf);
}
--
2.7.4

2021-02-03 01:19:19

by Jarkko Sakkinen

Subject: Re: [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid

On Tue, Feb 02, 2021 at 11:09:03PM +0100, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> In tpm2_del_space() the sessions are flushed by means of the tpm_chip
> operations. However the concerning operations pointer my already be NULL at
~~
may

What is "concerniog operations"? Unfamiliar term. Maybe just consistently
se chip->ops? Now you have also "tpm_chip operations" and chip->ops, in
addition to "concerning operations" in one paragraph commit message.

> this time in case that the chip has been unregistered (see
> tpm_chip_unregister() which calls tpm_del_char_device() which sets
> chip->ops to NULL).
> Before accessing chip->ops check if it is still valid. Skip flushing
> the sessions in this case.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>

Instead of cross referencing please describe the scenario (i.e.
the sequence of operations) of failure.

Fixes tag is also missing.

> ---
> drivers/char/tpm/tpm2-space.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 784b8b3..9a29a40 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -58,12 +58,17 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>
> void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
> {
> - mutex_lock(&chip->tpm_mutex);
> - if (!tpm_chip_start(chip)) {
> - tpm2_flush_sessions(chip, space);
> - tpm_chip_stop(chip);
> + down_read(&chip->ops_sem);
> + if (chip->ops) {
> + mutex_lock(&chip->tpm_mutex);
> + if (!tpm_chip_start(chip)) {
> + tpm2_flush_sessions(chip, space);
> + tpm_chip_stop(chip);
> + }
> + mutex_unlock(&chip->tpm_mutex);
> }
> - mutex_unlock(&chip->tpm_mutex);
> + up_read(&chip->ops_sem);
> +
> kfree(space->context_buf);
> kfree(space->session_buf);
> }
> --
> 2.7.4
>

/Jarkko

2021-02-03 14:26:34

by Lino Sanfilippo

Subject: Re: [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid

Hi,

On 03.02.21 02:17, Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 11:09:03PM +0100, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <[email protected]>
>>
>> In tpm2_del_space() the sessions are flushed by means of the tpm_chip
>> operations. However the concerning operations pointer my already be NULL at
> ~~
> may
>
> What is "concerniog operations"? Unfamiliar term. Maybe just consistently
> se chip->ops? Now you have also "tpm_chip operations" and chip->ops, in
> addition to "concerning operations" in one paragraph commit message.

'concerning' referred to 'operations pointer'. But yes, using multiple times
a different term for the same thing is quite confusing. I will fix this.

>> this time in case that the chip has been unregistered (see
>> tpm_chip_unregister() which calls tpm_del_char_device() which sets
>> chip->ops to NULL).
>> Before accessing chip->ops check if it is still valid. Skip flushing
>> the sessions in this case.
>>
>> Signed-off-by: Lino Sanfilippo <[email protected]>
>
> Instead of cross referencing please describe the scenario (i.e.
> the sequence of operations) of failure.
>
> Fixes tag is also missing.
>

Right, will add it.

Thanks,
Lino