[tpmdd-devel] TPM: Avoid reference to potentially freed memory
diff mbox

Message ID 1445545961-5620-1-git-send-email-christophe.jaillet@wanadoo.fr
State New
Headers show

Commit Message

Christophe JAILLET Oct. 22, 2015, 8:32 p.m. UTC
Reference to the 'np' node is dropped before dereferencing the 'sizep' and
'basep' pointers, which could by then point to junk if the node has been
freed.

Refactor code to call 'of_node_pup' later.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/char/tpm/tpm_of.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Oct. 27, 2015, 10:27 a.m. UTC | #1
On Fri, Oct 23, 2015 at 10:37:33AM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 22, 2015 at 10:32:41PM +0200, Christophe JAILLET wrote:
> > Reference to the 'np' node is dropped before dereferencing the 'sizep' and
> > 'basep' pointers, which could by then point to junk if the node has been
> > freed.
> > 
> > Refactor code to call 'of_node_pup' later.
> > 
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> LGTM.

Is there anyone able to provide Tested-by for this?

Christophe, were you able to reproduce the crash (insmod/rmmod couple
of times maybe?) and validate that it was gone after fixing the bug?

> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

------------------------------------------------------------------------------
Christophe JAILLET Oct. 29, 2015, 6:12 a.m. UTC | #2
Le 27/10/2015 11:27, Jarkko Sakkinen a écrit :
> On Fri, Oct 23, 2015 at 10:37:33AM +0300, Jarkko Sakkinen wrote:
>> On Thu, Oct 22, 2015 at 10:32:41PM +0200, Christophe JAILLET wrote:
>>> Reference to the 'np' node is dropped before dereferencing the 'sizep' and
>>> 'basep' pointers, which could by then point to junk if the node has been
>>> freed.
>>>
>>> Refactor code to call 'of_node_pup' later.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> LGTM.
> Is there anyone able to provide Tested-by for this?
>
> Christophe, were you able to reproduce the crash (insmod/rmmod couple
> of times maybe?) and validate that it was gone after fixing the bug?

Hi,
no, I never triggered the bug.
This is just something noticed while looking at potential issues related 
to incorrect use of 'of_node_pup'.
I only compile tested the patch.

Best regards,
CJ

>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> /Jarkko
>


------------------------------------------------------------------------------
Jarkko Sakkinen Oct. 29, 2015, 10:48 a.m. UTC | #3
On Thu, Oct 29, 2015 at 07:12:01AM +0100, Marion & Christophe JAILLET wrote:
> 
> 
> Le 27/10/2015 11:27, Jarkko Sakkinen a écrit :
> >On Fri, Oct 23, 2015 at 10:37:33AM +0300, Jarkko Sakkinen wrote:
> >>On Thu, Oct 22, 2015 at 10:32:41PM +0200, Christophe JAILLET wrote:
> >>>Reference to the 'np' node is dropped before dereferencing the 'sizep' and
> >>>'basep' pointers, which could by then point to junk if the node has been
> >>>freed.
> >>>
> >>>Refactor code to call 'of_node_pup' later.
> >>>
> >>>Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >>LGTM.
> >Is there anyone able to provide Tested-by for this?
> >
> >Christophe, were you able to reproduce the crash (insmod/rmmod couple
> >of times maybe?) and validate that it was gone after fixing the bug?
> 
> Hi,
> no, I never triggered the bug.
> This is just something noticed while looking at potential issues related to
> incorrect use of 'of_node_pup'.
> I only compile tested the patch.

The fix is so obvious that I see no reason not to include it. Thanks for
the good work.

> Best regards,
> CJ

/Jarkko

------------------------------------------------------------------------------
Jarkko Sakkinen Oct. 30, 2015, 11:38 a.m. UTC | #4
On Thu, Oct 29, 2015 at 12:48:44PM +0200, Jarkko Sakkinen wrote:
> On Thu, Oct 29, 2015 at 07:12:01AM +0100, Marion & Christophe JAILLET wrote:
> > 
> > 
> > Le 27/10/2015 11:27, Jarkko Sakkinen a écrit :
> > >On Fri, Oct 23, 2015 at 10:37:33AM +0300, Jarkko Sakkinen wrote:
> > >>On Thu, Oct 22, 2015 at 10:32:41PM +0200, Christophe JAILLET wrote:
> > >>>Reference to the 'np' node is dropped before dereferencing the 'sizep' and
> > >>>'basep' pointers, which could by then point to junk if the node has been
> > >>>freed.
> > >>>
> > >>>Refactor code to call 'of_node_pup' later.
> > >>>
> > >>>Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > >>LGTM.
> > >Is there anyone able to provide Tested-by for this?
> > >
> > >Christophe, were you able to reproduce the crash (insmod/rmmod couple
> > >of times maybe?) and validate that it was gone after fixing the bug?
> > 
> > Hi,
> > no, I never triggered the bug.
> > This is just something noticed while looking at potential issues related to
> > incorrect use of 'of_node_pup'.
> > I only compile tested the patch.
> 
> The fix is so obvious that I see no reason not to include it. Thanks for
> the good work.

I'm getting

$ git am ~/tmp/of-fix.patch 
Applying: TPM: Avoid reference to potentially freed memory
error: patch failed: drivers/char/tpm/tpm_of.c:53
error: drivers/char/tpm/tpm_of.c: patch does not apply
Patch failed at 0001 TPM: Avoid reference to potentially freed memory
The copy of the patch that failed is found in:
   /home/jsakkine/projects/tpm2/git/linux-tpmdd/.git/rebase-apply/patch
   When you have resolved this problem, run "git am --continue".
   If you prefer to skip this patch, run "git am --skip" instead.
   To restore the original branch and stop patching, run "git am
   --abort".

I'm applying this against Linus tree (4.3-rc7).

/Jarkko

------------------------------------------------------------------------------

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 1141456..570f30c 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -53,17 +53,18 @@  int read_log(struct tpm_bios_log *log)
 		goto cleanup_eio;
 	}
 
-	of_node_put(np);
 	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
 	if (!log->bios_event_log) {
 		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
 		       __func__);
+		of_node_put(np);
 		return -ENOMEM;
 	}
 
 	log->bios_event_log_end = log->bios_event_log + *sizep;
 
 	memcpy(log->bios_event_log, __va(*basep), *sizep);
+	of_node_put(np);
 
 	return 0;