| Message ID | 1479304573-13601-1-git-send-email-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/16/2016 07:26 PM, Stefan Berger wrote: > Check the bios_dir entry for NULL before accessing it. Currently > this crashes the driver when a TPM 2 is attached and the entries > are NULL. Thanks Stefan !! I think it would be good to also add Fixes in commit description here. Fixes: d660a91a1b9d (tpm: adds NULL check for securityfs pseudo files) Thanks & Regards, - Nayna > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > --- > drivers/char/tpm/tpm_eventlog.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c > index ebec4ac..fb603a7 100644 > --- a/drivers/char/tpm/tpm_eventlog.c > +++ b/drivers/char/tpm/tpm_eventlog.c > @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) > * This design ensures that open() either safely gets kref or fails. > */ > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > - inode = d_inode(chip->bios_dir[i]); > - inode_lock(inode); > - inode->i_private = NULL; > - inode_unlock(inode); > - securityfs_remove(chip->bios_dir[i]); > + if (chip->bios_dir[i]) { > + inode = d_inode(chip->bios_dir[i]); > + inode_lock(inode); > + inode->i_private = NULL; > + inode_unlock(inode); > + securityfs_remove(chip->bios_dir[i]); > + } > } > } > ------------------------------------------------------------------------------
On Wed, Nov 16, 2016 at 08:56:13AM -0500, Stefan Berger wrote: > Check the bios_dir entry for NULL before accessing it. Currently > this crashes the driver when a TPM 2 is attached and the entries > are NULL. > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/char/tpm/tpm_eventlog.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c > index ebec4ac..fb603a7 100644 > --- a/drivers/char/tpm/tpm_eventlog.c > +++ b/drivers/char/tpm/tpm_eventlog.c > @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) > * This design ensures that open() either safely gets kref or fails. > */ > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > - inode = d_inode(chip->bios_dir[i]); > - inode_lock(inode); > - inode->i_private = NULL; > - inode_unlock(inode); > - securityfs_remove(chip->bios_dir[i]); > + if (chip->bios_dir[i]) { > + inode = d_inode(chip->bios_dir[i]); > + inode_lock(inode); > + inode->i_private = NULL; > + inode_unlock(inode); > + securityfs_remove(chip->bios_dir[i]); > + } > } > } > -- > 2.4.3 /Jarkko ------------------------------------------------------------------------------
On Wed, Nov 16, 2016 at 08:28:30PM +0530, Nayna wrote: > > > On 11/16/2016 07:26 PM, Stefan Berger wrote: > > Check the bios_dir entry for NULL before accessing it. Currently > > this crashes the driver when a TPM 2 is attached and the entries > > are NULL. > > Thanks Stefan !! I think it would be good to also add Fixes in commit > description here. > > Fixes: d660a91a1b9d (tpm: adds NULL check for securityfs pseudo files) Not mandatory as this is not in the mainline tree but doesn't hurt either. /Jarkko > > Thanks & Regards, > - Nayna > > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > --- > > drivers/char/tpm/tpm_eventlog.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c > > index ebec4ac..fb603a7 100644 > > --- a/drivers/char/tpm/tpm_eventlog.c > > +++ b/drivers/char/tpm/tpm_eventlog.c > > @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) > > * This design ensures that open() either safely gets kref or fails. > > */ > > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > > - inode = d_inode(chip->bios_dir[i]); > > - inode_lock(inode); > > - inode->i_private = NULL; > > - inode_unlock(inode); > > - securityfs_remove(chip->bios_dir[i]); > > + if (chip->bios_dir[i]) { > > + inode = d_inode(chip->bios_dir[i]); > > + inode_lock(inode); > > + inode->i_private = NULL; > > + inode_unlock(inode); > > + securityfs_remove(chip->bios_dir[i]); > > + } > > } > > } > > > ------------------------------------------------------------------------------
On 11/16/2016 10:40 AM, Jarkko Sakkinen wrote: > On Wed, Nov 16, 2016 at 08:28:30PM +0530, Nayna wrote: >> >> On 11/16/2016 07:26 PM, Stefan Berger wrote: >>> Check the bios_dir entry for NULL before accessing it. Currently >>> this crashes the driver when a TPM 2 is attached and the entries >>> are NULL. >> Thanks Stefan !! I think it would be good to also add Fixes in commit >> description here. >> >> Fixes: d660a91a1b9d (tpm: adds NULL check for securityfs pseudo files) > Not mandatory as this is not in the mainline tree but doesn't hurt > either. I wished you could just merge this fix patch into Nayna's original patch -- makes bisecting the code a lot easier. Stefan > > /Jarkko > >> Thanks & Regards, >> - Nayna >> >>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> --- >>> drivers/char/tpm/tpm_eventlog.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c >>> index ebec4ac..fb603a7 100644 >>> --- a/drivers/char/tpm/tpm_eventlog.c >>> +++ b/drivers/char/tpm/tpm_eventlog.c >>> @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) >>> * This design ensures that open() either safely gets kref or fails. >>> */ >>> for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { >>> - inode = d_inode(chip->bios_dir[i]); >>> - inode_lock(inode); >>> - inode->i_private = NULL; >>> - inode_unlock(inode); >>> - securityfs_remove(chip->bios_dir[i]); >>> + if (chip->bios_dir[i]) { >>> + inode = d_inode(chip->bios_dir[i]); >>> + inode_lock(inode); >>> + inode->i_private = NULL; >>> + inode_unlock(inode); >>> + securityfs_remove(chip->bios_dir[i]); >>> + } >>> } >>> } >>> ------------------------------------------------------------------------------
On Wed, Nov 16, 2016 at 10:49:39AM -0500, Stefan Berger wrote: > On 11/16/2016 10:40 AM, Jarkko Sakkinen wrote: > > On Wed, Nov 16, 2016 at 08:28:30PM +0530, Nayna wrote: > > > > > > On 11/16/2016 07:26 PM, Stefan Berger wrote: > > > > Check the bios_dir entry for NULL before accessing it. Currently > > > > this crashes the driver when a TPM 2 is attached and the entries > > > > are NULL. > > > Thanks Stefan !! I think it would be good to also add Fixes in commit > > > description here. > > > > > > Fixes: d660a91a1b9d (tpm: adds NULL check for securityfs pseudo files) > > Not mandatory as this is not in the mainline tree but doesn't hurt > > either. > I wished you could just merge this fix patch into Nayna's original patch -- > makes bisecting the code a lot easier. > > Stefan Works for me. /Jarkko > > > > > /Jarkko > > > > > Thanks & Regards, > > > - Nayna > > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > > > --- > > > > drivers/char/tpm/tpm_eventlog.c | 12 +++++++----- > > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c > > > > index ebec4ac..fb603a7 100644 > > > > --- a/drivers/char/tpm/tpm_eventlog.c > > > > +++ b/drivers/char/tpm/tpm_eventlog.c > > > > @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) > > > > * This design ensures that open() either safely gets kref or fails. > > > > */ > > > > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > > > > - inode = d_inode(chip->bios_dir[i]); > > > > - inode_lock(inode); > > > > - inode->i_private = NULL; > > > > - inode_unlock(inode); > > > > - securityfs_remove(chip->bios_dir[i]); > > > > + if (chip->bios_dir[i]) { > > > > + inode = d_inode(chip->bios_dir[i]); > > > > + inode_lock(inode); > > > > + inode->i_private = NULL; > > > > + inode_unlock(inode); > > > > + securityfs_remove(chip->bios_dir[i]); > > > > + } > > > > } > > > > } > > > > > ------------------------------------------------------------------------------
On Wed, Nov 16, 2016 at 08:56:13AM -0500, Stefan Berger wrote: > Check the bios_dir entry for NULL before accessing it. Currently > this crashes the driver when a TPM 2 is attached and the entries > are NULL. Yep > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > - inode = d_inode(chip->bios_dir[i]); > - inode_lock(inode); > - inode->i_private = NULL; > - inode_unlock(inode); > - securityfs_remove(chip->bios_dir[i]); > + if (chip->bios_dir[i]) { Nope, this must be is_err_or_null, we store err ptrs in this array. Jason ------------------------------------------------------------------------------
On Wed, Nov 16, 2016 at 01:38:53PM -0700, Jason Gunthorpe wrote: > On Wed, Nov 16, 2016 at 08:56:13AM -0500, Stefan Berger wrote: > > Check the bios_dir entry for NULL before accessing it. Currently > > this crashes the driver when a TPM 2 is attached and the entries > > are NULL. > > Yep > > > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > > - inode = d_inode(chip->bios_dir[i]); > > - inode_lock(inode); > > - inode->i_private = NULL; > > - inode_unlock(inode); > > - securityfs_remove(chip->bios_dir[i]); > > + if (chip->bios_dir[i]) { > > Nope, this must be is_err_or_null, we store err ptrs in this array. " err: chip->bios_dir[cnt] = NULL; " There is assignment to NULL so this should be fine. /JArko ------------------------------------------------------------------------------
On Wed, Nov 16, 2016 at 02:06:33PM -0800, Jarkko Sakkinen wrote: > On Wed, Nov 16, 2016 at 01:38:53PM -0700, Jason Gunthorpe wrote: > > On Wed, Nov 16, 2016 at 08:56:13AM -0500, Stefan Berger wrote: > > > Check the bios_dir entry for NULL before accessing it. Currently > > > this crashes the driver when a TPM 2 is attached and the entries > > > are NULL. > > > > Yep > > > > > for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { > > > - inode = d_inode(chip->bios_dir[i]); > > > - inode_lock(inode); > > > - inode->i_private = NULL; > > > - inode_unlock(inode); > > > - securityfs_remove(chip->bios_dir[i]); > > > + if (chip->bios_dir[i]) { > > > > Nope, this must be is_err_or_null, we store err ptrs in this array. > > " > err: > chip->bios_dir[cnt] = NULL; > " > > There is assignment to NULL so this should be fine. Applied. Not yet squashed. I'll save that for the next week and possible other squashes to the point when I prepare the pull request. /Jarkko ------------------------------------------------------------------------------
On Wed, Nov 16, 2016 at 02:06:33PM -0800, Jarkko Sakkinen wrote: > > > > Nope, this must be is_err_or_null, we store err ptrs in this array. > > " > err: > chip->bios_dir[cnt] = NULL; > " > > There is assignment to NULL so this should be fine. Oh good, I'm glad that made it in Jason ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c index ebec4ac..fb603a7 100644 --- a/drivers/char/tpm/tpm_eventlog.c +++ b/drivers/char/tpm/tpm_eventlog.c @@ -455,10 +455,12 @@ void tpm_bios_log_teardown(struct tpm_chip *chip) * This design ensures that open() either safely gets kref or fails. */ for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) { - inode = d_inode(chip->bios_dir[i]); - inode_lock(inode); - inode->i_private = NULL; - inode_unlock(inode); - securityfs_remove(chip->bios_dir[i]); + if (chip->bios_dir[i]) { + inode = d_inode(chip->bios_dir[i]); + inode_lock(inode); + inode->i_private = NULL; + inode_unlock(inode); + securityfs_remove(chip->bios_dir[i]); + } } }
Check the bios_dir entry for NULL before accessing it. Currently this crashes the driver when a TPM 2 is attached and the entries are NULL. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- drivers/char/tpm/tpm_eventlog.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)