| Message ID | 1500309907-9357-1-git-send-email-alex.popov@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 17 Jul 2017, Alexander Popov wrote: > Add an assertion similar to "fasttop" check in GNU C Library allocator: > an object added to a singly linked freelist should not point to itself. > That helps to detect some double free errors (e.g. CVE-2017-2636) without > slub_debug and KASAN. Testing with hackbench doesn't show any noticeable > performance penalty. We are adding up "unnoticable performance penalties". This is used int both the critical allocation and free paths. Could this be VM_BUG_ON()? > > Signed-off-by: Alexander Popov <alex.popov@linux.com> > --- > mm/slub.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/slub.c b/mm/slub.c > index 1d3f983..a106939b 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -261,6 +261,7 @@ static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) > > static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) > { > + BUG_ON(object == fp); /* naive detection of double free or corruption */ > *(void **)(object + s->offset) = fp; > } > >
On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote: > Add an assertion similar to "fasttop" check in GNU C Library allocator: > an object added to a singly linked freelist should not point to itself. > That helps to detect some double free errors (e.g. CVE-2017-2636) without > slub_debug and KASAN. Testing with hackbench doesn't show any noticeable > performance penalty. > { > + BUG_ON(object == fp); /* naive detection of double free or corruption */ > *(void **)(object + s->offset) = fp; > } Is BUG() the best response to this situation? If it's a corruption, then yes, but if we spot a double-free, then surely we should WARN() and return without doing anything?
On Mon, 17 Jul 2017, Matthew Wilcox wrote: > On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote: > > Add an assertion similar to "fasttop" check in GNU C Library allocator: > > an object added to a singly linked freelist should not point to itself. > > That helps to detect some double free errors (e.g. CVE-2017-2636) without > > slub_debug and KASAN. Testing with hackbench doesn't show any noticeable > > performance penalty. > > > { > > + BUG_ON(object == fp); /* naive detection of double free or corruption */ > > *(void **)(object + s->offset) = fp; > > } > > Is BUG() the best response to this situation? If it's a corruption, then > yes, but if we spot a double-free, then surely we should WARN() and return > without doing anything? The double free debug checking already does the same thing in a more thourough way (this one only checks if the last free was the same address). So its duplicating a check that already exists. However, this one is always on.
On 17.07.2017 20:54, Matthew Wilcox wrote: > On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote: >> Add an assertion similar to "fasttop" check in GNU C Library allocator: >> an object added to a singly linked freelist should not point to itself. >> That helps to detect some double free errors (e.g. CVE-2017-2636) without >> slub_debug and KASAN. Testing with hackbench doesn't show any noticeable >> performance penalty. > >> { >> + BUG_ON(object == fp); /* naive detection of double free or corruption */ >> *(void **)(object + s->offset) = fp; >> } > > Is BUG() the best response to this situation? If it's a corruption, then > yes, but if we spot a double-free, then surely we should WARN() and return > without doing anything? Hello Matthew, Double-free leads to the memory corruption too, since the next two kmalloc() calls return the same address to their callers. And we can spot it early here. -- Alexander
Hello Christopher, Thanks for your reply. On 17.07.2017 21:04, Christopher Lameter wrote: > On Mon, 17 Jul 2017, Matthew Wilcox wrote: > >> On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote: >>> Add an assertion similar to "fasttop" check in GNU C Library allocator: >>> an object added to a singly linked freelist should not point to itself. >>> That helps to detect some double free errors (e.g. CVE-2017-2636) without >>> slub_debug and KASAN. Testing with hackbench doesn't show any noticeable >>> performance penalty. >> >>> { >>> + BUG_ON(object == fp); /* naive detection of double free or corruption */ >>> *(void **)(object + s->offset) = fp; >>> } >> >> Is BUG() the best response to this situation? If it's a corruption, then >> yes, but if we spot a double-free, then surely we should WARN() and return >> without doing anything? > > The double free debug checking already does the same thing in a more > thourough way (this one only checks if the last free was the same > address). So its duplicating a check that already exists. Yes, absolutely. Enabled slub_debug (or KASAN with its quarantine) can detect more double-free errors. But it introduces much bigger performance penalty and it's disabled by default. > However, this one is always on. Yes, I would propose to have this relatively cheap check enabled by default. I think it will block a good share of double-free errors. Currently it's really easy to turn such a double-free into use-after-free and exploit it, since, as I wrote, next two kmalloc() calls return the same address. So we could make exploiting harder for a relatively low price. Christopher, if I change BUG_ON() to VM_BUG_ON(), it will be disabled by default again, right? Best regards, Alexander
On Mon, Jul 17, 2017 at 12:01 PM, Alexander Popov <alex.popov@linux.com> wrote: > Hello Christopher, > > Thanks for your reply. > > On 17.07.2017 21:04, Christopher Lameter wrote: >> On Mon, 17 Jul 2017, Matthew Wilcox wrote: >> >>> On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote: >>>> Add an assertion similar to "fasttop" check in GNU C Library allocator: >>>> an object added to a singly linked freelist should not point to itself. >>>> That helps to detect some double free errors (e.g. CVE-2017-2636) without >>>> slub_debug and KASAN. Testing with hackbench doesn't show any noticeable >>>> performance penalty. >>> >>>> { >>>> + BUG_ON(object == fp); /* naive detection of double free or corruption */ >>>> *(void **)(object + s->offset) = fp; >>>> } >>> >>> Is BUG() the best response to this situation? If it's a corruption, then >>> yes, but if we spot a double-free, then surely we should WARN() and return >>> without doing anything? >> >> The double free debug checking already does the same thing in a more >> thourough way (this one only checks if the last free was the same >> address). So its duplicating a check that already exists. > > Yes, absolutely. Enabled slub_debug (or KASAN with its quarantine) can detect > more double-free errors. But it introduces much bigger performance penalty and > it's disabled by default. > >> However, this one is always on. > > Yes, I would propose to have this relatively cheap check enabled by default. I > think it will block a good share of double-free errors. Currently it's really > easy to turn such a double-free into use-after-free and exploit it, since, as I > wrote, next two kmalloc() calls return the same address. So we could make > exploiting harder for a relatively low price. > > Christopher, if I change BUG_ON() to VM_BUG_ON(), it will be disabled by default > again, right? Let's merge this with the proposed CONFIG_FREELIST_HARDENED, then the performance change is behind a config, and we gain the rest of the freelist protections at the same time: http://www.openwall.com/lists/kernel-hardening/2017/07/06/1 -Kees
On Mon, 17 Jul 2017, Alexander Popov wrote: > Christopher, if I change BUG_ON() to VM_BUG_ON(), it will be disabled by default > again, right? It will be enabled if the distro ships with VM debugging on by default.
On 17.07.2017 22:11, Kees Cook wrote: > On Mon, Jul 17, 2017 at 12:01 PM, Alexander Popov <alex.popov@linux.com> wrote: >> Hello Christopher, >> >> Thanks for your reply. >> >> On 17.07.2017 21:04, Christopher Lameter wrote: >>> On Mon, 17 Jul 2017, Matthew Wilcox wrote: >>> >>>> On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote: >>>>> Add an assertion similar to "fasttop" check in GNU C Library allocator: >>>>> an object added to a singly linked freelist should not point to itself. >>>>> That helps to detect some double free errors (e.g. CVE-2017-2636) without >>>>> slub_debug and KASAN. Testing with hackbench doesn't show any noticeable >>>>> performance penalty. >>>> >>>>> { >>>>> + BUG_ON(object == fp); /* naive detection of double free or corruption */ >>>>> *(void **)(object + s->offset) = fp; >>>>> } >>>> >>>> Is BUG() the best response to this situation? If it's a corruption, then >>>> yes, but if we spot a double-free, then surely we should WARN() and return >>>> without doing anything? >>> >>> The double free debug checking already does the same thing in a more >>> thourough way (this one only checks if the last free was the same >>> address). So its duplicating a check that already exists. >> >> Yes, absolutely. Enabled slub_debug (or KASAN with its quarantine) can detect >> more double-free errors. But it introduces much bigger performance penalty and >> it's disabled by default. >> >>> However, this one is always on. >> >> Yes, I would propose to have this relatively cheap check enabled by default. I >> think it will block a good share of double-free errors. Currently it's really >> easy to turn such a double-free into use-after-free and exploit it, since, as I >> wrote, next two kmalloc() calls return the same address. So we could make >> exploiting harder for a relatively low price. >> >> Christopher, if I change BUG_ON() to VM_BUG_ON(), it will be disabled by default >> again, right? > > Let's merge this with the proposed CONFIG_FREELIST_HARDENED, then the > performance change is behind a config, and we gain the rest of the > freelist protections at the same time: > > http://www.openwall.com/lists/kernel-hardening/2017/07/06/1 Hello Kees, If I change BUG_ON() to VM_BUG_ON(), this check will work at least on Fedora since it has CONFIG_DEBUG_VM enabled. Debian based distros have this option disabled. Do you like that more than having this check under CONFIG_FREELIST_HARDENED? If you insist on putting this check under CONFIG_FREELIST_HARDENED, should I rebase onto your patch and send again? Best regards, Alexander
On Tue, Jul 18, 2017 at 12:56 PM, Alexander Popov <alex.popov@linux.com> wrote: > On 17.07.2017 22:11, Kees Cook wrote: >> On Mon, Jul 17, 2017 at 12:01 PM, Alexander Popov <alex.popov@linux.com> wrote: >>> Hello Christopher, >>> >>> Thanks for your reply. >>> >>> On 17.07.2017 21:04, Christopher Lameter wrote: >>>> On Mon, 17 Jul 2017, Matthew Wilcox wrote: >>>> >>>>> On Mon, Jul 17, 2017 at 07:45:07PM +0300, Alexander Popov wrote: >>>>>> Add an assertion similar to "fasttop" check in GNU C Library allocator: >>>>>> an object added to a singly linked freelist should not point to itself. >>>>>> That helps to detect some double free errors (e.g. CVE-2017-2636) without >>>>>> slub_debug and KASAN. Testing with hackbench doesn't show any noticeable >>>>>> performance penalty. >>>>> >>>>>> { >>>>>> + BUG_ON(object == fp); /* naive detection of double free or corruption */ >>>>>> *(void **)(object + s->offset) = fp; >>>>>> } >>>>> >>>>> Is BUG() the best response to this situation? If it's a corruption, then >>>>> yes, but if we spot a double-free, then surely we should WARN() and return >>>>> without doing anything? >>>> >>>> The double free debug checking already does the same thing in a more >>>> thourough way (this one only checks if the last free was the same >>>> address). So its duplicating a check that already exists. >>> >>> Yes, absolutely. Enabled slub_debug (or KASAN with its quarantine) can detect >>> more double-free errors. But it introduces much bigger performance penalty and >>> it's disabled by default. >>> >>>> However, this one is always on. >>> >>> Yes, I would propose to have this relatively cheap check enabled by default. I >>> think it will block a good share of double-free errors. Currently it's really >>> easy to turn such a double-free into use-after-free and exploit it, since, as I >>> wrote, next two kmalloc() calls return the same address. So we could make >>> exploiting harder for a relatively low price. >>> >>> Christopher, if I change BUG_ON() to VM_BUG_ON(), it will be disabled by default >>> again, right? >> >> Let's merge this with the proposed CONFIG_FREELIST_HARDENED, then the >> performance change is behind a config, and we gain the rest of the >> freelist protections at the same time: >> >> http://www.openwall.com/lists/kernel-hardening/2017/07/06/1 > > Hello Kees, > > If I change BUG_ON() to VM_BUG_ON(), this check will work at least on Fedora > since it has CONFIG_DEBUG_VM enabled. Debian based distros have this option > disabled. Do you like that more than having this check under > CONFIG_FREELIST_HARDENED? I think there are two issues: first, this should likely be under CONFIG_FREELIST_HARDENED since Christoph hasn't wanted to make these changes enabled by default (if I'm understanding his earlier review comments to me). The second issue is what to DO when a double-free is discovered. Is there any way to make it safe/survivable? If not, I think it should just be BUG_ON(). If it can be made safe, then likely a WARN_ONCE and do whatever is needed to prevent the double-free. > If you insist on putting this check under CONFIG_FREELIST_HARDENED, should I > rebase onto your patch and send again? That would be preferred for me -- I'd like to have both checks. :) -Kees
On 18.07.2017 23:04, Kees Cook wrote: > On Tue, Jul 18, 2017 at 12:56 PM, Alexander Popov <alex.popov@linux.com> wrote: >> On 17.07.2017 22:11, Kees Cook wrote: >>> Let's merge this with the proposed CONFIG_FREELIST_HARDENED, then the >>> performance change is behind a config, and we gain the rest of the >>> freelist protections at the same time: >>> >>> http://www.openwall.com/lists/kernel-hardening/2017/07/06/1 >> >> Hello Kees, >> >> If I change BUG_ON() to VM_BUG_ON(), this check will work at least on Fedora >> since it has CONFIG_DEBUG_VM enabled. Debian based distros have this option >> disabled. Do you like that more than having this check under >> CONFIG_FREELIST_HARDENED? > > I think there are two issues: first, this should likely be under > CONFIG_FREELIST_HARDENED since Christoph hasn't wanted to make these > changes enabled by default (if I'm understanding his earlier review > comments to me). Ok, I'll rebase onto FREELIST_HARDENED and test it all together. > The second issue is what to DO when a double-free is > discovered. Is there any way to make it safe/survivable? If not, I > think it should just be BUG_ON(). If it can be made safe, then likely > a WARN_ONCE and do whatever is needed to prevent the double-free. Please correct me if I'm wrong. It seems to me that double-free is a dangerous situation that indicates some serious kernel bug (which might be maliciously exploited). So I would not trust / rely on the process which experiences a double-free error in the kernel mode. But I guess the reaction to it should depend on the Linux kernel policy of handling faults. Is it defined explicitly? Anyway, if we try to mitigate the effect from a double-free error _here_ (for example, skip putting the duplicated object to the freelist), I think we should do the same for other cases of double-free and memory corruptions. >> If you insist on putting this check under CONFIG_FREELIST_HARDENED, should I >> rebase onto your patch and send again? > > That would be preferred for me -- I'd like to have both checks. :) Ok. Best regards, Alexander
On Tue, 18 Jul 2017, Kees Cook wrote: > I think there are two issues: first, this should likely be under > CONFIG_FREELIST_HARDENED since Christoph hasn't wanted to make these > changes enabled by default (if I'm understanding his earlier review > comments to me). The second issue is what to DO when a double-free is > discovered. Is there any way to make it safe/survivable? If not, I The simple thing is to not free the object when you discover this. That is what the existing debugging code does. But you do not have an easy way to fail at the point in the code that is patched here.
diff --git a/mm/slub.c b/mm/slub.c index 1d3f983..a106939b 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -261,6 +261,7 @@ static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) { + BUG_ON(object == fp); /* naive detection of double free or corruption */ *(void **)(object + s->offset) = fp; }
Add an assertion similar to "fasttop" check in GNU C Library allocator: an object added to a singly linked freelist should not point to itself. That helps to detect some double free errors (e.g. CVE-2017-2636) without slub_debug and KASAN. Testing with hackbench doesn't show any noticeable performance penalty. Signed-off-by: Alexander Popov <alex.popov@linux.com> --- mm/slub.c | 1 + 1 file changed, 1 insertion(+)