Re: [kernel-hardening] [PATCH 22/23] usercopy: split user-controlled slabs to separate caches
On Mon, Jun 19, 2017 at 9:47 PM, Eric Biggers wrote:
> On Mon, Jun 19, 2017 at 04:36:36PM -0700, Kees Cook wrote:
>> From: David Windsor
>>
>> Some userspace APIs (e.g. ipc, seq_file) provide precise control over
>> the size of kernel kmallocs, which provides a trivial way to perform
>> heap overflow attacks where the attacker must control neighboring
>> allocations of a specific size. Instead, move these APIs into their own
>> cache so they cannot interfere with standard kmallocs. This is enabled
>> with CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC.
>>
>> This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY_SLABS
>> code in the last public patch of grsecurity/PaX based on my understanding
>> of the code. Changes or omissions from the original code are mine and
>> don't reflect the original grsecurity/PaX code.
>>
>> Signed-off-by: David Windsor
>> [kees: added SLAB_NO_MERGE flag to allow split of future no-merge Kconfig]
>> Signed-off-by: Kees Cook
>> ---
>> fs/seq_file.c| 2 +-
>> include/linux/gfp.h | 9 -
>> include/linux/slab.h | 12
>> ipc/msgutil.c| 5 +++--
>> mm/slab.h| 3 ++-
>> mm/slab_common.c | 29 -
>> security/Kconfig | 12
>> 7 files changed, 66 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/seq_file.c b/fs/seq_file.c
>> index dc7c2be963ed..5caa58a19bdc 100644
>> --- a/fs/seq_file.c
>> +++ b/fs/seq_file.c
>> @@ -25,7 +25,7 @@ static void seq_set_overflow(struct seq_file *m)
>>
>> static void *seq_buf_alloc(unsigned long size)
>> {
>> - return kvmalloc(size, GFP_KERNEL);
>> + return kvmalloc(size, GFP_KERNEL | GFP_USERCOPY);
>> }
>>
>
> Also forgot to mention the obvious: there are way more places where
> GFP_USERCOPY
> would need to be (or should be) used. Helper functions like memdup_user() and
> memdup_user_nul() would be the obvious ones. And just a random example, some
> of
> the keyrings syscalls (callable with no privileges) do a kmalloc() with
> user-controlled contents and size.
Looking again at how grsecurity uses it, they have some of those call
sites a couple more (keyctl, char/mem, kcore, memdup_user). Getting
the facility in place at all is a good first step, IMO.
>
> So I think this by itself needs its own patch series.
Sounds reasonable.
-Kees
--
Kees Cook
Pixel Security
Re: [kernel-hardening] [PATCH 22/23] usercopy: split user-controlled slabs to separate caches
On Mon, Jun 19, 2017 at 04:36:36PM -0700, Kees Cook wrote:
> From: David Windsor
>
> Some userspace APIs (e.g. ipc, seq_file) provide precise control over
> the size of kernel kmallocs, which provides a trivial way to perform
> heap overflow attacks where the attacker must control neighboring
> allocations of a specific size. Instead, move these APIs into their own
> cache so they cannot interfere with standard kmallocs. This is enabled
> with CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC.
>
> This patch is modified from Brad Spengler/PaX Team's PAX_USERCOPY_SLABS
> code in the last public patch of grsecurity/PaX based on my understanding
> of the code. Changes or omissions from the original code are mine and
> don't reflect the original grsecurity/PaX code.
>
> Signed-off-by: David Windsor
> [kees: added SLAB_NO_MERGE flag to allow split of future no-merge Kconfig]
> Signed-off-by: Kees Cook
> ---
> fs/seq_file.c| 2 +-
> include/linux/gfp.h | 9 -
> include/linux/slab.h | 12
> ipc/msgutil.c| 5 +++--
> mm/slab.h| 3 ++-
> mm/slab_common.c | 29 -
> security/Kconfig | 12
> 7 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index dc7c2be963ed..5caa58a19bdc 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -25,7 +25,7 @@ static void seq_set_overflow(struct seq_file *m)
>
> static void *seq_buf_alloc(unsigned long size)
> {
> - return kvmalloc(size, GFP_KERNEL);
> + return kvmalloc(size, GFP_KERNEL | GFP_USERCOPY);
> }
>
Also forgot to mention the obvious: there are way more places where GFP_USERCOPY
would need to be (or should be) used. Helper functions like memdup_user() and
memdup_user_nul() would be the obvious ones. And just a random example, some of
the keyrings syscalls (callable with no privileges) do a kmalloc() with
user-controlled contents and size.
So I think this by itself needs its own patch series.
Eric
Re: [kernel-hardening] [PATCH 22/23] usercopy: split user-controlled slabs to separate caches
On Mon, Jun 19, 2017 at 04:36:36PM -0700, Kees Cook wrote: > From: David Windsor > > Some userspace APIs (e.g. ipc, seq_file) provide precise control over > the size of kernel kmallocs, which provides a trivial way to perform > heap overflow attacks where the attacker must control neighboring > allocations of a specific size. Instead, move these APIs into their own > cache so they cannot interfere with standard kmallocs. This is enabled > with CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC. > This is a logically separate change which IMO should be its own patch, not just patch 22/23. Also, is this really just about heap overflows? I thought the main purpose of separate heaps is to make it more difficult to exploit use-after-frees, since anything allocating an object from heap A cannot overwrite freed memory in heap B. (At least, not at the SLAB level; it may still be done at the page level.) > diff --git a/include/linux/gfp.h b/include/linux/gfp.h > index a89d37e8b387..ff4f4a698ad0 100644 > --- a/include/linux/gfp.h > +++ b/include/linux/gfp.h > @@ -45,6 +45,7 @@ struct vm_area_struct; > #else > #define ___GFP_NOLOCKDEP 0 > #endif > +#define ___GFP_USERCOPY 0x400u > /* If the above are modified, __GFP_BITS_SHIFT may need updating */ > > /* > @@ -83,12 +84,17 @@ struct vm_area_struct; > * node with no fallbacks or placement policy enforcements. > * > * __GFP_ACCOUNT causes the allocation to be accounted to kmemcg. > + * > + * __GFP_USERCOPY indicates that the page will be explicitly copied to/from > + * userspace, and may be allocated from a separate kmalloc pool. > + * > */ The "page", or the allocation? It's only for slab objects, is it not? More importantly, the purpose of this needs to be clearly documented; otherwise people won't know what this is and whether they should/need to use it or not. - Eric
