* Re: Regarding have kfree() (and related) set the pointer to NULL too @ 2019-06-27 10:19 Gote, Nitin R 2019-06-27 11:45 ` Vegard Nossum 0 siblings, 1 reply; 6+ messages in thread From: Gote, Nitin R @ 2019-06-27 10:19 UTC (permalink / raw) To: Kees Cook; +Cc: kernel-hardening [-- Attachment #1: Type: text/plain, Size: 1165 bytes --] Hi, I'm looking into "have kfree() (and related) set the pointer to NULL too" task. As per my understanding, I did below changes : Could you please provide some points on below ways ? diff --git a/mm/slab.c b/mm/slab.c index f7117ad..a6e3d1b 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3754,6 +3754,7 @@ void kfree(const void *objp) debug_check_no_obj_freed(objp, c->object_size); __cache_free(c, (void *)objp, _RET_IP_); local_irq_restore(flags); + objp = NULL; } EXPORT_SYMBOL(kfree); diff --git a/mm/slob.c b/mm/slob.c index 84aefd9..dcdb815 100644 --- a/mm/slob.c +++ b/mm/slob.c @@ -523,6 +523,8 @@ void kfree(const void *block) slob_free(m, *m + align); } else __free_pages(sp, compound_order(sp)); + + block = NULL; } EXPORT_SYMBOL(kfree); diff --git a/mm/slub.c b/mm/slub.c index cd04dbd..7cc400a 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3947,6 +3947,8 @@ void kfree(const void *x) return; } slab_free(page->slab_cache, page, object, NULL, 1, _RET_IP_); + + x = NULL; } EXPORT_SYMBOL(kfree); [-- Attachment #2: Type: text/html, Size: 5093 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Regarding have kfree() (and related) set the pointer to NULL too 2019-06-27 10:19 Regarding have kfree() (and related) set the pointer to NULL too Gote, Nitin R @ 2019-06-27 11:45 ` Vegard Nossum 2019-06-27 16:21 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Vegard Nossum @ 2019-06-27 11:45 UTC (permalink / raw) To: Gote, Nitin R; +Cc: Kees Cook, kernel-hardening On Thu, 27 Jun 2019 at 12:23, Gote, Nitin R <nitin.r.gote@intel.com> wrote: > Hi, > > I’m looking into “have kfree() (and related) set the pointer to NULL too” task. > > As per my understanding, I did below changes : > > Could you please provide some points on below ways ? > @@ -3754,6 +3754,7 @@ void kfree(const void *objp) > debug_check_no_obj_freed(objp, c->object_size); > __cache_free(c, (void *)objp, _RET_IP_); > local_irq_restore(flags); > + objp = NULL; > > } This will not do anything, since the assignment happens to the local variable inside kfree() rather than to the original expression that was passed to it as an argument. Consider that the code in the caller looks like this: void *x = kmalloc(...); kfree(x); pr_info("x = %p\n", x); this will still print "x = (some non-NULL address)" because the variable 'x' in the caller still retains its original value. You could try wrapping kfree() in a C macro, something like #define kfree(x) real_kfree(x); (x) = NULL; but using proper C macro best practices (like putting do {} while (0) around it, etc.). It's probably easier to play with this in a simple userspace program first. Vegard ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regarding have kfree() (and related) set the pointer to NULL too 2019-06-27 11:45 ` Vegard Nossum @ 2019-06-27 16:21 ` Kees Cook 2019-07-06 20:52 ` Gote, Nitin R 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2019-06-27 16:21 UTC (permalink / raw) To: Vegard Nossum; +Cc: Gote, Nitin R, kernel-hardening On Thu, Jun 27, 2019 at 01:45:06PM +0200, Vegard Nossum wrote: > On Thu, 27 Jun 2019 at 12:23, Gote, Nitin R <nitin.r.gote@intel.com> wrote: > > Hi, > > > > I’m looking into “have kfree() (and related) set the pointer to NULL too” task. > > > > As per my understanding, I did below changes : > > > > Could you please provide some points on below ways ? > > @@ -3754,6 +3754,7 @@ void kfree(const void *objp) > > debug_check_no_obj_freed(objp, c->object_size); > > __cache_free(c, (void *)objp, _RET_IP_); > > local_irq_restore(flags); > > + objp = NULL; > > > > } > > This will not do anything, since the assignment happens to the local > variable inside kfree() rather than to the original expression that > was passed to it as an argument. > > Consider that the code in the caller looks like this: > > void *x = kmalloc(...); > kfree(x); > pr_info("x = %p\n", x); > > this will still print "x = (some non-NULL address)" because the > variable 'x' in the caller still retains its original value. > > You could try wrapping kfree() in a C macro, something like > > #define kfree(x) real_kfree(x); (x) = NULL; Right, though we want to avoid silent double-evaluation, so we have to do some macro tricks. I suspect the starting point is something like: #define kfree(x) \ do { \ typeof(x) *ptr = &(x); \ real_kfree(*ptr); \ *ptr = NULL; \ } while (0) However, there are a non-zero number of places in the kernel where kfree() is used on things that are not simple memory references, like function return values, or copies of the actual reference: kfree(get_my_allocation(foo)); or previous = something->allocation; ... kfree(prevous) So the larger work is figuring out how to gracefully deal with those using a reasonable API, or through refactoring. However, before getting too far, it's worth going though past use-after-free vulnerabilities to figure out how many would have been rendered harmless (NULL deref instead of UaF) with this change. Has this been studied before, etc. With this information it's easier to decide if the benefit of this refactoring is worth the work to do it. -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: Regarding have kfree() (and related) set the pointer to NULL too 2019-06-27 16:21 ` Kees Cook @ 2019-07-06 20:52 ` Gote, Nitin R 2019-07-17 10:01 ` Gote, Nitin R 0 siblings, 1 reply; 6+ messages in thread From: Gote, Nitin R @ 2019-07-06 20:52 UTC (permalink / raw) To: Kees Cook, Vegard Nossum; +Cc: kernel-hardening > -----Original Message----- > From: Kees Cook [mailto:keescook@chromium.org] > Sent: Thursday, June 27, 2019 9:52 PM > To: Vegard Nossum <vegard.nossum@gmail.com> > Cc: Gote, Nitin R <nitin.r.gote@intel.com>; kernel- > hardening@lists.openwall.com > Subject: Re: Regarding have kfree() (and related) set the pointer to NULL too > > On Thu, Jun 27, 2019 at 01:45:06PM +0200, Vegard Nossum wrote: > > On Thu, 27 Jun 2019 at 12:23, Gote, Nitin R <nitin.r.gote@intel.com> wrote: > > > Hi, > > > > > > I’m looking into “have kfree() (and related) set the pointer to NULL too” > task. > > > > > > As per my understanding, I did below changes : > > > > > > Could you please provide some points on below ways ? > > > @@ -3754,6 +3754,7 @@ void kfree(const void *objp) > > > debug_check_no_obj_freed(objp, c->object_size); > > > __cache_free(c, (void *)objp, _RET_IP_); > > > local_irq_restore(flags); > > > + objp = NULL; > > > > > > } > > > > This will not do anything, since the assignment happens to the local > > variable inside kfree() rather than to the original expression that > > was passed to it as an argument. > > > > Consider that the code in the caller looks like this: > > > > void *x = kmalloc(...); > > kfree(x); > > pr_info("x = %p\n", x); > > > > this will still print "x = (some non-NULL address)" because the > > variable 'x' in the caller still retains its original value. > > > > You could try wrapping kfree() in a C macro, something like > > > > #define kfree(x) real_kfree(x); (x) = NULL; > > Right, though we want to avoid silent double-evaluation, so we have to do > some macro tricks. I suspect the starting point is something like: > > #define kfree(x) \ > do { \ > typeof(x) *ptr = &(x); \ > real_kfree(*ptr); \ > *ptr = NULL; \ > } while (0) > > However, there are a non-zero number of places in the kernel where kfree() > is used on things that are not simple memory references, like function return > values, or copies of the actual reference: > > kfree(get_my_allocation(foo)); > > or > > previous = something->allocation; > ... > kfree(prevous) > > So the larger work is figuring out how to gracefully deal with those using a > reasonable API, or through refactoring. > > However, before getting too far, it's worth going though past use-after-free > vulnerabilities to figure out how many would have been rendered harmless > (NULL deref instead of UaF) with this change. Has this been studied before, > etc. With this information it's easier to decide if the benefit of this refactoring > is worth the work to do it. > As per my understanding from above comment is that, First need to check below changes are harmless or not using tool like KASAN (KASAN report). > #define kfree(x) \ > do { \ > typeof(x) *ptr = &(x); \ > real_kfree(*ptr); \ > *ptr = NULL; \ > } while (0) If I misunderstood this, Could you please elaborate this? As I'm not able to conclude on this completely. -- Nitin Gote. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: Regarding have kfree() (and related) set the pointer to NULL too 2019-07-06 20:52 ` Gote, Nitin R @ 2019-07-17 10:01 ` Gote, Nitin R 2019-07-22 17:10 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Gote, Nitin R @ 2019-07-17 10:01 UTC (permalink / raw) To: Kees Cook, Vegard Nossum; +Cc: kernel-hardening > > -----Original Message----- > > From: Kees Cook [mailto:keescook@chromium.org] > > Sent: Thursday, June 27, 2019 9:52 PM > > To: Vegard Nossum <vegard.nossum@gmail.com> > > Cc: Gote, Nitin R <nitin.r.gote@intel.com>; kernel- > > hardening@lists.openwall.com > > Subject: Re: Regarding have kfree() (and related) set the pointer to > > NULL too > > > > On Thu, Jun 27, 2019 at 01:45:06PM +0200, Vegard Nossum wrote: > > > On Thu, 27 Jun 2019 at 12:23, Gote, Nitin R <nitin.r.gote@intel.com> > wrote: > > > > Hi, > > > > > > > > I’m looking into “have kfree() (and related) set the pointer to NULL too” > > task. > > > > > > > > As per my understanding, I did below changes : > > > > > > > > Could you please provide some points on below ways ? > > > > @@ -3754,6 +3754,7 @@ void kfree(const void *objp) > > > > debug_check_no_obj_freed(objp, c->object_size); > > > > __cache_free(c, (void *)objp, _RET_IP_); > > > > local_irq_restore(flags); > > > > + objp = NULL; > > > > > > > > } > > > > > > This will not do anything, since the assignment happens to the local > > > variable inside kfree() rather than to the original expression that > > > was passed to it as an argument. > > > > > > Consider that the code in the caller looks like this: > > > > > > void *x = kmalloc(...); > > > kfree(x); > > > pr_info("x = %p\n", x); > > > > > > this will still print "x = (some non-NULL address)" because the > > > variable 'x' in the caller still retains its original value. > > > > > > You could try wrapping kfree() in a C macro, something like > > > > > > #define kfree(x) real_kfree(x); (x) = NULL; > > > > Right, though we want to avoid silent double-evaluation, so we have to > > do some macro tricks. I suspect the starting point is something like: > > > > #define kfree(x) \ > > do { \ > > typeof(x) *ptr = &(x); \ > > real_kfree(*ptr); \ > > *ptr = NULL; \ > > } while (0) > > > > However, there are a non-zero number of places in the kernel where > > kfree() is used on things that are not simple memory references, like > > function return values, or copies of the actual reference: > > > > kfree(get_my_allocation(foo)); > > We have not found any clue or compiler extension on this case to know x is value or variable. There are around 300 such cases in kernel where kfree() is used on function return values. One proposal is we can refactor the kfree() call by assigning it to local variable so that we can overcome the compilation issue (error: lvalue required as unary & operand). Please let us know that we should proceed with this proposal or you (or anyone) have other proposals. > > or > > > > previous = something->allocation; > > ... > > kfree(previous) > > As of now, we don't have any idea to overcome this case. So, we are keeping this for later stage. In case anyone has idea on this then let me know, we are ready to work on this. > > So the larger work is figuring out how to gracefully deal with those > > using a reasonable API, or through refactoring. > > > > However, before getting too far, it's worth going though past > > use-after-free vulnerabilities to figure out how many would have been > > rendered harmless (NULL deref instead of UaF) with this change. Has > > this been studied before, etc. With this information it's easier to > > decide if the benefit of this refactoring is worth the work to do it. > > Thanks, Nitin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regarding have kfree() (and related) set the pointer to NULL too 2019-07-17 10:01 ` Gote, Nitin R @ 2019-07-22 17:10 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2019-07-22 17:10 UTC (permalink / raw) To: Gote, Nitin R; +Cc: Vegard Nossum, kernel-hardening On Wed, Jul 17, 2019 at 10:01:21AM +0000, Gote, Nitin R wrote: > > > -----Original Message----- > > > From: Kees Cook [mailto:keescook@chromium.org] > > > Sent: Thursday, June 27, 2019 9:52 PM > > > To: Vegard Nossum <vegard.nossum@gmail.com> > > > Cc: Gote, Nitin R <nitin.r.gote@intel.com>; kernel- > > > hardening@lists.openwall.com > > > Subject: Re: Regarding have kfree() (and related) set the pointer to > > > NULL too > > > > > > On Thu, Jun 27, 2019 at 01:45:06PM +0200, Vegard Nossum wrote: > > > > On Thu, 27 Jun 2019 at 12:23, Gote, Nitin R <nitin.r.gote@intel.com> > > wrote: > > > > > Hi, > > > > > > > > > > I’m looking into “have kfree() (and related) set the pointer to NULL too” > > > task. > > > > > > > > > > As per my understanding, I did below changes : > > > > > > > > > > Could you please provide some points on below ways ? > > > > > @@ -3754,6 +3754,7 @@ void kfree(const void *objp) > > > > > debug_check_no_obj_freed(objp, c->object_size); > > > > > __cache_free(c, (void *)objp, _RET_IP_); > > > > > local_irq_restore(flags); > > > > > + objp = NULL; > > > > > > > > > > } > > > > > > > > This will not do anything, since the assignment happens to the local > > > > variable inside kfree() rather than to the original expression that > > > > was passed to it as an argument. > > > > > > > > Consider that the code in the caller looks like this: > > > > > > > > void *x = kmalloc(...); > > > > kfree(x); > > > > pr_info("x = %p\n", x); > > > > > > > > this will still print "x = (some non-NULL address)" because the > > > > variable 'x' in the caller still retains its original value. > > > > > > > > You could try wrapping kfree() in a C macro, something like > > > > > > > > #define kfree(x) real_kfree(x); (x) = NULL; > > > > > > Right, though we want to avoid silent double-evaluation, so we have to > > > do some macro tricks. I suspect the starting point is something like: > > > > > > #define kfree(x) \ > > > do { \ > > > typeof(x) *ptr = &(x); \ > > > real_kfree(*ptr); \ > > > *ptr = NULL; \ > > > } while (0) > > > > > > However, there are a non-zero number of places in the kernel where > > > kfree() is used on things that are not simple memory references, like > > > function return values, or copies of the actual reference: > > > > > > kfree(get_my_allocation(foo)); > > > > > We have not found any clue or compiler extension on this case to know x > is value or variable. Do you mean find a way to detect if this is a function return value? Yeah, this seems like a needed step. > There are around 300 such cases in kernel where kfree() is used on function return values. > > One proposal is we can refactor the kfree() call by assigning it to local variable so that > we can overcome the compilation issue (error: lvalue required as unary & operand). What were you thinking for an implementation? Universally using a local variable doesn't gain very much (since you're just setting the local variable NULL, not the one that lives on outside the call to kfree()). > Please let us know that we should proceed with this proposal or you (or anyone) > have other proposals. > > > > or > > > > > > previous = something->allocation; > > > ... > > > kfree(previous) > > > > > As of now, we don't have any idea to overcome this case. > So, we are keeping this for later stage. It sounds like this may not be possible with the existing APIs -- maybe a compiler feature is needed to do the variable tracking (and also the "was this a function return value?" check). > > In case anyone has idea on this then let me know, > we are ready to work on this. > > > > So the larger work is figuring out how to gracefully deal with those > > > using a reasonable API, or through refactoring. > > > > > > However, before getting too far, it's worth going though past > > > use-after-free vulnerabilities to figure out how many would have been > > > rendered harmless (NULL deref instead of UaF) with this change. Has > > > this been studied before, etc. With this information it's easier to > > > decide if the benefit of this refactoring is worth the work to do it. This part is still worth checking: is there a real-world benefit to this idea? (It seems like it should make some classes of UAF go away, but it'd be nice to back this up with some "proof" from existing prior exploits.) -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-22 17:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-27 10:19 Regarding have kfree() (and related) set the pointer to NULL too Gote, Nitin R 2019-06-27 11:45 ` Vegard Nossum 2019-06-27 16:21 ` Kees Cook 2019-07-06 20:52 ` Gote, Nitin R 2019-07-17 10:01 ` Gote, Nitin R 2019-07-22 17:10 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).