[patch] x86, microcode, AMD: use after free in free_cache()

[patch] x86, microcode, AMD: use after free in free_cache()

[Dan Carpenter]

list_for_each_entry_reverse() dereferences the iterator, but we already
freed it.  I don't see a reason that this has to be done in reverse
order so I've changed it to use list_for_each_entry_safe().

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is only needed on linux-next.  Sorry, it turns out that I don't
have any AMD systems right now, so I have not tested this.

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 5511216..7720ff5 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -150,9 +150,9 @@ static void update_cache(struct ucode_patch *new_patch)
 
 static void free_cache(void)
 {
-	struct ucode_patch *p;
+	struct ucode_patch *p, *tmp;
 
-	list_for_each_entry_reverse(p, &pcache, plist) {
+	list_for_each_entry_safe(p, tmp, &pcache, plist) {
 		__list_del(p->plist.prev, p->plist.next);
 		kfree(p->data);
 		kfree(p);

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Borislav Petkov]

On Wed, Sep 05, 2012 at 03:30:42PM +0300, Dan Carpenter wrote:
> list_for_each_entry_reverse() dereferences the iterator, but we already
> freed it.

Wait a sec, we assign the iterator in each iteration of the loop, right?

And if so, I don't see a problem: we derefence a new element each time
and *then* free it...

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Dan Carpenter]

On Wed, Sep 05, 2012 at 03:20:57PM +0200, Borislav Petkov wrote:
> On Wed, Sep 05, 2012 at 03:30:42PM +0300, Dan Carpenter wrote:
> > list_for_each_entry_reverse() dereferences the iterator, but we already
> > freed it.
> 
> Wait a sec, we assign the iterator in each iteration of the loop, right?
> 
> And if so, I don't see a problem: we derefence a new element each time
> and *then* free it...

The dereference happens inside the assignment.  That's actually the
reason why we have the the _safe() version of the macro.

regards,
dan carpenter

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Borislav Petkov]

On Wed, Sep 05, 2012 at 04:42:03PM -0700, Dan Carpenter wrote:
> The dereference happens inside the assignment.

Yes, this:

#define list_for_each_entry_reverse(pos, head, member)			\
	for (pos = list_entry((head)->prev, typeof(*pos), member);	\
	     &pos->member != (head); 	\					<--- DEREF.
	     pos = list_entry(pos->member.prev, typeof(*pos), member))

but we kfree pos aka p after the deref and in the next iteration p
becomes the list entry of the next list element, AFAICT.

> That's actually the reason why we have the the _safe() version of the
> macro.

_safe, the way I see it, is for concurrent list manipulations and at the
point we free the cache, I don't see us concurrently manipulating that
list.

So, sorry, but I don't see the problem.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Dan Carpenter]

On Thu, Sep 06, 2012 at 02:01:45PM +0200, Borislav Petkov wrote:
> On Wed, Sep 05, 2012 at 04:42:03PM -0700, Dan Carpenter wrote:
> > The dereference happens inside the assignment.
> 
> Yes, this:
> 
> #define list_for_each_entry_reverse(pos, head, member)			\
> 	for (pos = list_entry((head)->prev, typeof(*pos), member);	\
> 	     &pos->member != (head); 	\					<--- DEREF.

No.  That's not what I'm talking about.  (And also that's not a
dereference, it just gives you the address of the struct member).

> 	     pos = list_entry(pos->member.prev, typeof(*pos), member))
                              ^^^^^
Here is the dereference.  We have already freed "pos" at this point.

> 
> but we kfree pos aka p after the deref and in the next iteration p
> becomes the list entry of the next list element, AFAICT.
> 
> > That's actually the reason why we have the the _safe() version of the
> > macro.
> 
> _safe, the way I see it, is for concurrent list manipulations and at the
> point we free the cache, I don't see us concurrently manipulating that
> list.
> 

GAR GAR GAR!  STOP! NO!  I've seen this before where people remove
locking code and change to using the _safe() version of the
list_for_each macros.  The _safe() version has *NOTHING* to do with
concurency.  It is for if we are freeing a list element.

regards,
dan carpenter

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Borislav Petkov]

On Thu, Sep 06, 2012 at 05:30:48AM -0700, Dan Carpenter wrote:
> > #define list_for_each_entry_reverse(pos, head, member)			\
> > 	for (pos = list_entry((head)->prev, typeof(*pos), member);	\
> > 	     &pos->member != (head); 	\					<--- DEREF.
> 
> No.  That's not what I'm talking about.  (And also that's not a
> dereference, it just gives you the address of the struct member).
> 
> > 	     pos = list_entry(pos->member.prev, typeof(*pos), member))
>                               ^^^^^
> Here is the dereference.  We have already freed "pos" at this point.

Ok, I see it now, thanks for pointing it out exactly.

One last thing remains: why didn't I hit this during testing at all?
Timings, or some other out-of-order x86 reason I'm unable to see now?

> GAR GAR GAR! STOP! NO! I've seen this before where people remove
> locking code and change to using the _safe() version of the
> list_for_each macros. The _safe() version has *NOTHING* to do with
> concurency. It is for if we are freeing a list element.

Ok.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Dan Carpenter]

On Thu, Sep 06, 2012 at 03:06:53PM +0200, Borislav Petkov wrote:
> On Thu, Sep 06, 2012 at 05:30:48AM -0700, Dan Carpenter wrote:
> > > #define list_for_each_entry_reverse(pos, head, member)			\
> > > 	for (pos = list_entry((head)->prev, typeof(*pos), member);	\
> > > 	     &pos->member != (head); 	\					<--- DEREF.
> > 
> > No.  That's not what I'm talking about.  (And also that's not a
> > dereference, it just gives you the address of the struct member).
> > 
> > > 	     pos = list_entry(pos->member.prev, typeof(*pos), member))
> >                               ^^^^^
> > Here is the dereference.  We have already freed "pos" at this point.
> 
> Ok, I see it now, thanks for pointing it out exactly.
> 
> One last thing remains: why didn't I hit this during testing at all?
> Timings, or some other out-of-order x86 reason I'm unable to see now?
> 

You wouldn't see it unless something called kmalloc() on another CPU
and reused the freed memory.  Or if you had CONFIG_DEBUG_SLAB
enabled then I believe that will poison freed memory immediately.

(Btw, I haven't tested this code either before or after my patch
because I don't have any AMD systems).

regards,
dan carpenter

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Borislav Petkov]

On Thu, Sep 06, 2012 at 07:04:01AM -0700, Dan Carpenter wrote:
> You wouldn't see it unless something called kmalloc() on another CPU
> and reused the freed memory. Or if you had CONFIG_DEBUG_SLAB enabled
> then I believe that will poison freed memory immediately.

Oh, I see.

Also, I could probably see it if I do

	p = NULL;

to the iterator AFAICT.

> (Btw, I haven't tested this code either before or after my patch
> because I don't have any AMD systems).

I'll run it when I get back from my vacation next week.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [patch] x86, microcode, AMD: use after free in free_cache()

[Borislav Petkov]

On Wed, Sep 05, 2012 at 03:30:42PM +0300, Dan Carpenter wrote:
> list_for_each_entry_reverse() dereferences the iterator, but we already
> freed it.  I don't see a reason that this has to be done in reverse
> order so I've changed it to use list_for_each_entry_safe().
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This is only needed on linux-next.  Sorry, it turns out that I don't
> have any AMD systems right now, so I have not tested this.

Ok, just ran it and it looks good.

Acked-by: Borislav Petkov <borislav.petkov@amd.com>

@hpa, this one should go ontop of tip/x86/microcode.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551