[PATCH 1/2] x86/boot: fix KASL when memmap range manipulation is used

[PATCH 1/2] x86/boot: fix KASL when memmap range manipulation is used

[Julian Stecklina]

From: Julian Stecklina <jsteckli@amazon.de>

When the user passes a memmap=<size>%<offset>-<oldtype>+<newtype>
parameter to the kernel to reclassify some memory, this information is
ignored during the randomization of the kernel base address. This in
turn leads to cases where the kernel is unpacked to memory regions that
the user marked as reserved.

Fix this situation to avoid any memory region for KASLR that is
reclassified.

Fixes: ef61f8a340fd6d49df6b367785743febc47320c1 ("x86/boot/e820: Implement a range manipulation operator")

Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
---
 arch/x86/boot/compressed/kaslr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 9ed9709..5657e34 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -155,6 +155,12 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
 	case '#':
 	case '$':
 	case '!':
+		/*
+		 * % would need some more complex parsing, because regions might
+		 * actually become usable for KASLR, but the simple way of
+		 * ignoring anything that is mentioned in % works for now.
+		 */
+	case '%':
 		*start = memparse(p + 1, &p);
 		return 0;
 	case '@':
-- 
2.7.4

[PATCH 2/2] x86/boot: increase maximum number of avoided KASLR regions

[Julian Stecklina]

From: Julian Stecklina <jsteckli@amazon.de>

The boot code has a limit of 4 "non-standard" regions to avoid for
KASLR. This limit is easy to reach when supplying memmap= parameters to
the kernel. In this case, KASLR would be disabled.

Increase the limit to avoid turning off KASLR even when the user is
heavily manipulating the memory map.

Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
---
 arch/x86/boot/compressed/kaslr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 5657e34..f078d60 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -92,8 +92,8 @@ struct mem_vector {
 	unsigned long long size;
 };
 
-/* Only supporting at most 4 unusable memmap regions with kaslr */
-#define MAX_MEMMAP_REGIONS	4
+/* Only supporting at most this many unusable memmap regions with kaslr */
+#define MAX_MEMMAP_REGIONS	16
 
 static bool memmap_too_large;
 
@@ -213,7 +213,7 @@ static void mem_avoid_memmap(char *str)
 		i++;
 	}
 
-	/* More than 4 memmaps, fail kaslr */
+	/* Can't store all regions, fail kaslr */
 	if ((i >= MAX_MEMMAP_REGIONS) && str)
 		memmap_too_large = true;
 }
-- 
2.7.4

Re: [PATCH 2/2] x86/boot: increase maximum number of avoided KASLR regions

[Borislav Petkov]

On Wed, Jan 30, 2019 at 05:40:03PM +0100, Julian Stecklina wrote:
> From: Julian Stecklina <jsteckli@amazon.de>
> 
> The boot code has a limit of 4 "non-standard" regions to avoid for
> KASLR. This limit is easy to reach when supplying memmap= parameters to
> the kernel. In this case, KASLR would be disabled.
> 
> Increase the limit to avoid turning off KASLR even when the user is
> heavily manipulating the memory map.
> 
> Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
> ---
>  arch/x86/boot/compressed/kaslr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 5657e34..f078d60 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -92,8 +92,8 @@ struct mem_vector {
>  	unsigned long long size;
>  };
>  
> -/* Only supporting at most 4 unusable memmap regions with kaslr */
> -#define MAX_MEMMAP_REGIONS	4
> +/* Only supporting at most this many unusable memmap regions with kaslr */
> +#define MAX_MEMMAP_REGIONS	16
>  
>  static bool memmap_too_large;
>  
> @@ -213,7 +213,7 @@ static void mem_avoid_memmap(char *str)
>  		i++;
>  	}
>  
> -	/* More than 4 memmaps, fail kaslr */
> +	/* Can't store all regions, fail kaslr */
>  	if ((i >= MAX_MEMMAP_REGIONS) && str)
>  		memmap_too_large = true;
>  }
> -- 

Lemme add some of the folks from
f28442497b5caf7bf573ade22a7f8d3559e3ef56 to Cc.

It all looks arbitrary to me: first 4 unusable memmap regions, this
patch raises it to 16. Why are we even imposing such a limit?

Hmm.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/boot: increase maximum number of avoided KASLR regions

[Julian Stecklina]

Borislav Petkov <bp@alien8.de> writes:

>> @@ -213,7 +213,7 @@ static void mem_avoid_memmap(char *str)
>>  		i++;
>>  	}
>>  
>> -	/* More than 4 memmaps, fail kaslr */
>> +	/* Can't store all regions, fail kaslr */
>>  	if ((i >= MAX_MEMMAP_REGIONS) && str)
>>  		memmap_too_large = true;
>>  }
>> -- 
>
> Lemme add some of the folks from
> f28442497b5caf7bf573ade22a7f8d3559e3ef56 to Cc.
>
> It all looks arbitrary to me: first 4 unusable memmap regions, this
> patch raises it to 16. Why are we even imposing such a limit?

Because at this point, we are not in a good position to handle an
unlimited amount of regions.

As for the choice of "16", I took our usecase and multiplied it by two.
FWIW, this could be even larger.

Julian

Re: [PATCH 2/2] x86/boot: increase maximum number of avoided KASLR regions

[Borislav Petkov]

On Wed, Feb 06, 2019 at 01:50:57PM +0100, Julian Stecklina wrote:
> Because at this point, we are not in a good position to handle an
> unlimited amount of regions.

We could save only the regions which are ok to kaslr into. And we do,
apparently:

static struct slot_area slot_areas[MAX_SLOT_AREA];

but I guess there was a reason to do the mem_avoid thing too instead of
collecting only OK ranges directly. Maybe Kees will know.

> As for the choice of "16", I took our usecase and multiplied it by two.
> FWIW, this could be even larger.

Because our kernel is not fat enough huh?

Btw, you missed a spot:

static unsigned long find_random_phys_addr(unsigned long minimum,
                                           unsigned long image_size)
{
        /* Check if we had too many memmaps. */
        if (memmap_too_large) {
                debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n");
								^^^^^^^^^^^^^
-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/boot: increase maximum number of avoided KASLR regions

[Kees Cook]

On Wed, Feb 6, 2019 at 2:18 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Feb 06, 2019 at 01:50:57PM +0100, Julian Stecklina wrote:
> > Because at this point, we are not in a good position to handle an
> > unlimited amount of regions.
>
> We could save only the regions which are ok to kaslr into. And we do,
> apparently:
>
> static struct slot_area slot_areas[MAX_SLOT_AREA];
>
> but I guess there was a reason to do the mem_avoid thing too instead of
> collecting only OK ranges directly. Maybe Kees will know.

Originally, there weren't a lot of things that needed to be avoided
and physical memory was relatively consecutive, so adding complexity
here didn't make sense.

I'm fine adjusting all this to do things better. Ultimately, we're
still walking two lists to process their intersection.

> > As for the choice of "16", I took our usecase and multiplied it by two.
> > FWIW, this could be even larger.
>
> Because our kernel is not fat enough huh?

Eh, it's just in the boot stub. ;)

-- 
Kees Cook

Re: [PATCH 2/2] x86/boot: increase maximum number of avoided KASLR regions

[Borislav Petkov]

On Wed, Feb 06, 2019 at 03:29:06PM +0000, Kees Cook wrote:
> I'm fine adjusting all this to do things better. Ultimately, we're
> still walking two lists to process their intersection.

I'm wondering if we could start with a single range including all memory
and then keep exluding until we're done and then feed those remaining
ranges to slots_fetch_random(). So that mem_avoid[] is not needed
anymore.

Probably need to look for the devil in the detail first.

> Eh, it's just in the boot stub. ;)

They always say something like that. :-)

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2] x86/boot: fix KASL when memmap range manipulation is used

[Baoquan He]

On 01/30/19 at 05:40pm, Julian Stecklina wrote:
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 9ed9709..5657e34 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -155,6 +155,12 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>  	case '#':
>  	case '$':
>  	case '!':
> +		/*
> +		 * % would need some more complex parsing, because regions might
> +		 * actually become usable for KASLR, but the simple way of
> +		 * ignoring anything that is mentioned in % works for now.
> +		 */

This seems to make thing more complicated even though have to. One
concern is whether we need to check the oldtype|newtype , e.g
oldtype=reserverd, newtype=RAM, is it possible to set like that?

Thanks
Baoquan

> +	case '%':
>  		*start = memparse(p + 1, &p);
>  		return 0;
>  	case '@':
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] x86/boot: fix KASL when memmap range manipulation is used

[Julian Stecklina]

Baoquan He <bhe@redhat.com> writes:

> On 01/30/19 at 05:40pm, Julian Stecklina wrote:
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 9ed9709..5657e34 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -155,6 +155,12 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size)
>>  	case '#':
>>  	case '$':
>>  	case '!':
>> +		/*
>> +		 * % would need some more complex parsing, because regions might
>> +		 * actually become usable for KASLR, but the simple way of
>> +		 * ignoring anything that is mentioned in % works for now.
>> +		 */
>
> This seems to make thing more complicated even though have to. One
> concern is whether we need to check the oldtype|newtype , e.g
> oldtype=reserverd, newtype=RAM, is it possible to set like that?

With the above patch the boot code will avoid using any region targeted
by % for KASLR. This does mean regions that are changed to be usable via
% are not taken into account.

Julian