Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[zerons]

Hi,

In slub.c(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/slub.c?h=v5.4.19#n305),
for SLAB_FREELIST_HARDENED, an extra detection of the double free bug has been added.

This patch can (maybe only) detect something like this: kfree(a) kfree(a).
However, it does nothing when another process calls kfree(b) between the two kfree above.

The problem is, if the panic_on_oops option is not set(Ubuntu 16.04/18.04 default option),
for a bug which kfree an object twice in a row, if another process can preempt the process
triggered this bug and then call kmalloc() to get the object, the patch doesn't work.

Case 0: failure race
Process A:
	kfree(a)
	kfree(a)
the patch could terminate Process A.

Case 1: race done
Process A:
	kfree(a)
Process B:
	kmalloc() -> a
Process A:
	kfree(a)
the patch does nothing.

The attacker can check the return status of process A to see if the race is done.

Without this extra detection, the kernel could be unstable while the attacker
trying to do the race.
In my opinion, this patch can somehow help attacker exploit this kind of bugs
more reliable.

Best Regards,

Re: Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[Andrey Konovalov]

On Thu, Feb 13, 2020 at 4:43 PM zerons <zeronsaxm@gmail.com> wrote:
>
> Hi,
>
> In slub.c(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/slub.c?h=v5.4.19#n305),
> for SLAB_FREELIST_HARDENED, an extra detection of the double free bug has been added.
>
> This patch can (maybe only) detect something like this: kfree(a) kfree(a).
> However, it does nothing when another process calls kfree(b) between the two kfree above.
>
> The problem is, if the panic_on_oops option is not set(Ubuntu 16.04/18.04 default option),
> for a bug which kfree an object twice in a row, if another process can preempt the process
> triggered this bug and then call kmalloc() to get the object, the patch doesn't work.
>
> Case 0: failure race
> Process A:
>         kfree(a)
>         kfree(a)
> the patch could terminate Process A.
>
> Case 1: race done
> Process A:
>         kfree(a)
> Process B:
>         kmalloc() -> a
> Process A:
>         kfree(a)
> the patch does nothing.
>
> The attacker can check the return status of process A to see if the race is done.
>
> Without this extra detection, the kernel could be unstable while the attacker
> trying to do the race.
> In my opinion, this patch can somehow help attacker exploit this kind of bugs
> more reliable.

+Alexander Popov, who is the author of the double free check in
SLAB_FREELIST_HARDENED.

Ah, so as long as the double free happens in a user process context,
you can retry triggering it until you succeed in winning the race to
reallocate the object (without causing slab freelist corruption, as it
would have had happened before SLAB_FREELIST_HARDENED). Nice idea!

Re: Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[Kees Cook]

On Mon, Feb 17, 2020 at 04:15:44PM +0100, Andrey Konovalov wrote:
> On Thu, Feb 13, 2020 at 4:43 PM zerons <zeronsaxm@gmail.com> wrote:
> >
> > Hi,
> >
> > In slub.c(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/slub.c?h=v5.4.19#n305),
> > for SLAB_FREELIST_HARDENED, an extra detection of the double free bug has been added.
> >
> > This patch can (maybe only) detect something like this: kfree(a) kfree(a).
> > However, it does nothing when another process calls kfree(b) between the two kfree above.
> >
> > The problem is, if the panic_on_oops option is not set(Ubuntu 16.04/18.04 default option),
> > for a bug which kfree an object twice in a row, if another process can preempt the process
> > triggered this bug and then call kmalloc() to get the object, the patch doesn't work.
> >
> > Case 0: failure race
> > Process A:
> >         kfree(a)
> >         kfree(a)
> > the patch could terminate Process A.
> >
> > Case 1: race done
> > Process A:
> >         kfree(a)
> > Process B:
> >         kmalloc() -> a
> > Process A:
> >         kfree(a)
> > the patch does nothing.
> >
> > The attacker can check the return status of process A to see if the race is done.
> >
> > Without this extra detection, the kernel could be unstable while the attacker
> > trying to do the race.

The check was just for the trivial case. It was an inexpensive check,
but was never designed to be a robust double-free defense.

> > In my opinion, this patch can somehow help attacker exploit this kind of bugs
> > more reliable.

Why do you think this makes races easier to win?

> +Alexander Popov, who is the author of the double free check in
> SLAB_FREELIST_HARDENED.
> 
> Ah, so as long as the double free happens in a user process context,
> you can retry triggering it until you succeed in winning the race to
> reallocate the object (without causing slab freelist corruption, as it
> would have had happened before SLAB_FREELIST_HARDENED). Nice idea!

Do you see improvements that could be made here?

-- 
Kees Cook

Re: Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[zerons]

> 
>>> In my opinion, this patch can somehow help attacker exploit this kind of bugs
>>> more reliable.
> 
> Why do you think this makes races easier to win?
> 

Sorry, not to make the races easier, but to make the exploitations
more reliable.

>> +Alexander Popov, who is the author of the double free check in
>> SLAB_FREELIST_HARDENED.
>>
>> Ah, so as long as the double free happens in a user process context,
>> you can retry triggering it until you succeed in winning the race to
>> reallocate the object (without causing slab freelist corruption, as it
>> would have had happened before SLAB_FREELIST_HARDENED). Nice idea!
> 
> Do you see improvements that could be made here?
> 

Could we use BUG_ON() only when panic_on_oops is set?

Re: Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[Alexander Popov]

Hello!

Thanks for adding me to this discussion.
Let me also add Jann Horn.

On 17.02.2020 18:15, Andrey Konovalov wrote:
> On Thu, Feb 13, 2020 at 4:43 PM zerons <zeronsaxm@gmail.com> wrote:
>> In slub.c(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/mm/slub.c?h=v5.4.19#n305),
>> for SLAB_FREELIST_HARDENED, an extra detection of the double free bug has been added.
>>
>> This patch can (maybe only) detect something like this: kfree(a) kfree(a).
>> However, it does nothing when another process calls kfree(b) between the two kfree above.

Yes, that's correct.

>> The problem is, if the panic_on_oops option is not set(Ubuntu 16.04/18.04 default option),
>> for a bug which kfree an object twice in a row, if another process can preempt the process
>> triggered this bug and then call kmalloc() to get the object, the patch doesn't work.

In theory, that is true.

However, let me show a counterexample from practice.

I developed this check after I exploited CVE-2017-2636 (race condition causing
double free). Please see the detailed write-up about the exploit:
https://a13xp0p0v.github.io/2017/03/24/CVE-2017-2636.html

There was a linked list with data buffers, and one of these buffers was added to
the list twice. Double free happened when the driver cleaned up its resources
and freed the buffers in this list. So double kfree() happened quite close to
each other.

I spent a lot of time trying to insert some kmalloc() between these kfree(), but
didn't succeed. That is difficult because slab caches are per-CPU, and heap
spray on other CPUs doesn't overwrite the needed kernel address.

The vulnerable kernel task didn't call scheduler between double kfree(). I
didn't manage to preempt it. But I solved that trouble by spraying _after_
double kfree().

>> Without this extra detection, the kernel could be unstable while the attacker
>> trying to do the race.

Could you bring more details? Which kind of instability do you mean?

When I did heap spray with sk_buffs after double kfree(), I got two sk_buff
items with sk_buff.head pointing to the same memory. Receiving one sk_buff
created use-after-free on another one. That is how double free turns into
use-after-free.

The check which we discuss now breaks that method.

Best regards,
Alexander

Re: Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[zerons]

> There was a linked list with data buffers, and one of these buffers was added to
> the list twice. Double free happened when the driver cleaned up its resources
> and freed the buffers in this list. So double kfree() happened quite close to
> each other.
> 
> I spent a lot of time trying to insert some kmalloc() between these kfree(), but

I did this too. :D

> didn't succeed. That is difficult because slab caches are per-CPU, and heap
> spray on other CPUs doesn't overwrite the needed kernel address.
> 
> The vulnerable kernel task didn't call scheduler between double kfree(). I
> didn't manage to preempt it. But I solved that trouble by spraying _after_
> double kfree().
> 
>>> Without this extra detection, the kernel could be unstable while the attacker
>>> trying to do the race.
> 
> Could you bring more details? Which kind of instability do you mean?
> 

Unlike cve-2017-2636 kind of double free bugs, assume that we have
another double free bug that would be stopped by this patch.

Now, the problem is, do we really have this kind of bugs that we could
win the race? I am not sure for now. Here is an example in Jann Horn's
latest article I read days ago. Though this example is not good enough
because the hlist operations may crash the kernel before double kfree(),
we should notice that some codes still can give us a chance.

(https://googleprojectzero.blogspot.com/2020/02/mitigations-are-attack-surface-too.html)
there could be a double free.
================================================================================
  spin_lock_irqsave(&table->pid_map_lock, irqsave_flags);
  hlist_for_each_entry(descr, &table->pid_map[hash_key], pid_map_node) {
    if (pid == descr->task->pid) {
      target_task_descr = descr;
      break;
    }
  }
  spin_unlock_irqrestore(&table->pid_map_lock, irqsave_flags);
================================================================================
The victim object is found in a hlist, and that's all we do in this critical section.
Later, it gets freed.

"As long as the double free happens in a user process context, you can
retry triggering it until you succeed in winning the race to reallocate
the object (without causing slab freelist corruption, as it would have
had happened before SLAB_FREELIST_HARDENED)." Andrey Konovalov said.

This patch does work for cve-2017-2636 case, it is barely impossible to win the
race. My concern is based on an assumption: we do have a double kfree() bug and
we can win the race.

Best regards,

Re: Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[Alexander Popov]

On 19.02.2020 16:43, zerons wrote:
> This patch does work for cve-2017-2636 case, it is barely impossible to win the
> race. My concern is based on an assumption: we do have a double kfree() bug and
> we can win the race.

Yes, I agree that the double-free check in CONFIG_SLAB_FREELIST_HARDENED can be
bypassed in some cases by winning the race and inserting kmalloc() between kfree().

But I *don't* agree that this double-free check can help the attacker.

Without this check in CONFIG_SLAB_FREELIST_HARDENED, double-free exploitation is
always easier, since the attacker has no need to race at all. In the write-up
about CVE-2017-2636 exploit [1] I showed how to do heap spray *after*
double-free (kfree-kfree-kmalloc-kmalloc).

Best regards,
Alexander

[1]: https://a13xp0p0v.github.io/2017/03/24/CVE-2017-2636.html

Re: Maybe inappropriate use BUG_ON() in CONFIG_SLAB_FREELIST_HARDENED

[zerons]

On 2/27/20 19:28, Alexander Popov wrote:
> On 19.02.2020 16:43, zerons wrote:
>> This patch does work for cve-2017-2636 case, it is barely impossible to win the
>> race. My concern is based on an assumption: we do have a double kfree() bug and
>> we can win the race.
> 
> Yes, I agree that the double-free check in CONFIG_SLAB_FREELIST_HARDENED can be
> bypassed in some cases by winning the race and inserting kmalloc() between kfree().
> 
> But I *don't* agree that this double-free check can help the attacker.
> 
> Without this check in CONFIG_SLAB_FREELIST_HARDENED, double-free exploitation is
> always easier, since the attacker has no need to race at all. In the write-up
> about CVE-2017-2636 exploit [1] I showed how to do heap spray *after*
> double-free (kfree-kfree-kmalloc-kmalloc).
> 

I thought the freelist obfuscation[1] and prefecth next pointer[2]
may block this method(kfree-kfree-kmalloc-kmalloc), and the
prefetch_freepointer() should've stopped the 2nd kmalloc().

Today, I did some tests on Ubuntu 18.04 with kernel 5.3.18,
without your patch.

Here is the code. It writes something to modprobe_path for debugging.
After the 2nd kmalloc() return, ptr0 == ptr1 is true, which means
the attacker could have two objects point to same memory area.

Although the system now is quite fragile: the next kmalloc()
would trigger do_general_protection() since the c->freelist is
something like 0x4141414141414141, the attacker still can win.

=======================================================================
#define TARGET_SIZE	0x1000
static int __init test_init(void)
{
	char *ptr0, *ptr1, *ptr2;
	char *path_addr;
	size_t l = 0;

	path_addr = (char *)kallsyms_lookup_name("modprobe_path");
	while (1) {
		if (!*(path_addr+l))
			break;
		l++;
	}
	l += 8 - (l%8);

	ptr0 = kmalloc(TARGET_SIZE, GFP_KERNEL);
	*(unsigned long *)(path_addr+l) = (unsigned long)ptr0;
	kfree(ptr0);
	*(unsigned long *)(path_addr+l+8) = *(unsigned long *)ptr0;

	*(unsigned long *)(path_addr+l+0x10) = (unsigned long)ptr0;
	kfree(ptr0);
	*(unsigned long *)(path_addr+l+0x18) = *(unsigned long *)ptr0;

	ptr0 = kmalloc(TARGET_SIZE, GFP_KERNEL);
	*(unsigned long *)(path_addr+l+0x20) = (unsigned long)ptr0;
	*(unsigned long *)(path_addr+l+0x28) = *(unsigned long *)ptr0;

	ptr1 = kmalloc(TARGET_SIZE, GFP_KERNEL);
	*(unsigned long *)(path_addr+l+0x30) = (unsigned long)ptr1;
	*(unsigned long *)(path_addr+l+0x38) = *(unsigned long *)ptr0;

#if 0
	ptr2 = kmalloc(TARGET_SIZE, GFP_KERNEL);
	*(unsigned long *)(path_addr+l+0x40) = (unsigned long)ptr2;
	*(unsigned long *)(path_addr+l+0x48) = *(unsigned long *)ptr0;
#endif

	return 0;
}
==========================================================================


[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2482ddec
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0ad9500e1