Re: [PATCH v4 1/4] x86/boot/KASLR: Wrap e820 entries walking code into new function process_e820_entries()

[Kees Cook]

On Sun, Jul 9, 2017 at 5:37 AM, Baoquan He  wrote:
> The original function process_e820_entry() only takes care of each
> e820 entry passed.
>
> And move the E820_TYPE_RAM checking logic into process_e820_entries().
>
> And remove the redundent local variable 'addr' definition in
> find_random_phys_addr().
>
> Signed-off-by: Baoquan He 

Acked-by: Kees Cook 

-Kees

> ---
>  arch/x86/boot/compressed/kaslr.c | 38 +-
>  1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c 
> b/arch/x86/boot/compressed/kaslr.c
> index 91f27ab970ef..1485f48aeda1 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -488,10 +488,6 @@ static void process_e820_entry(struct boot_e820_entry 
> *entry,
> unsigned long start_orig, end;
> struct boot_e820_entry cur_entry;
>
> -   /* Skip non-RAM entries. */
> -   if (entry->type != E820_TYPE_RAM)
> -   return;
> -
> /* On 32-bit, ignore entries entirely above our maximum. */
> if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
> return;
> @@ -562,12 +558,29 @@ static void process_e820_entry(struct boot_e820_entry 
> *entry,
> }
>  }
>
> -static unsigned long find_random_phys_addr(unsigned long minimum,
> -  unsigned long image_size)
> +static void process_e820_entries(unsigned long minimum,
> +unsigned long image_size)
>  {
> int i;
> -   unsigned long addr;
> +   struct boot_e820_entry *entry;
> +
> +   /* Verify potential e820 positions, appending to slots list. */
> +   for (i = 0; i < boot_params->e820_entries; i++) {
> +   entry = _params->e820_table[i];
> +   /* Skip non-RAM entries. */
> +   if (entry->type != E820_TYPE_RAM)
> +   continue;
> +   process_e820_entry(entry, minimum, image_size);
> +   if (slot_area_index == MAX_SLOT_AREA) {
> +   debug_putstr("Aborted e820 scan (slot_areas 
> full)!\n");
> +   break;
> +   }
> +   }
> +}
>
> +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 e820 scan (more than 4 memmap= 
> args)!\n");
> @@ -577,16 +590,7 @@ static unsigned long find_random_phys_addr(unsigned long 
> minimum,
> /* Make sure minimum is aligned. */
> minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
>
> -   /* Verify potential e820 positions, appending to slots list. */
> -   for (i = 0; i < boot_params->e820_entries; i++) {
> -   process_e820_entry(_params->e820_table[i], minimum,
> -  image_size);
> -   if (slot_area_index == MAX_SLOT_AREA) {
> -   debug_putstr("Aborted e820 scan (slot_areas 
> full)!\n");
> -   break;
> -   }
> -   }
> -
> +   process_e820_entries(minimum, image_size);
> return slots_fetch_random();
>  }
>
> --
> 2.5.5
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v4 1/4] x86/boot/KASLR: Wrap e820 entries walking code into new function process_e820_entries()

[Kees Cook]

On Sun, Jul 9, 2017 at 5:37 AM, Baoquan He  wrote:
> The original function process_e820_entry() only takes care of each
> e820 entry passed.
>
> And move the E820_TYPE_RAM checking logic into process_e820_entries().
>
> And remove the redundent local variable 'addr' definition in
> find_random_phys_addr().
>
> Signed-off-by: Baoquan He 

Acked-by: Kees Cook 

-Kees

> ---
>  arch/x86/boot/compressed/kaslr.c | 38 +-
>  1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c 
> b/arch/x86/boot/compressed/kaslr.c
> index 91f27ab970ef..1485f48aeda1 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -488,10 +488,6 @@ static void process_e820_entry(struct boot_e820_entry 
> *entry,
> unsigned long start_orig, end;
> struct boot_e820_entry cur_entry;
>
> -   /* Skip non-RAM entries. */
> -   if (entry->type != E820_TYPE_RAM)
> -   return;
> -
> /* On 32-bit, ignore entries entirely above our maximum. */
> if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
> return;
> @@ -562,12 +558,29 @@ static void process_e820_entry(struct boot_e820_entry 
> *entry,
> }
>  }
>
> -static unsigned long find_random_phys_addr(unsigned long minimum,
> -  unsigned long image_size)
> +static void process_e820_entries(unsigned long minimum,
> +unsigned long image_size)
>  {
> int i;
> -   unsigned long addr;
> +   struct boot_e820_entry *entry;
> +
> +   /* Verify potential e820 positions, appending to slots list. */
> +   for (i = 0; i < boot_params->e820_entries; i++) {
> +   entry = _params->e820_table[i];
> +   /* Skip non-RAM entries. */
> +   if (entry->type != E820_TYPE_RAM)
> +   continue;
> +   process_e820_entry(entry, minimum, image_size);
> +   if (slot_area_index == MAX_SLOT_AREA) {
> +   debug_putstr("Aborted e820 scan (slot_areas 
> full)!\n");
> +   break;
> +   }
> +   }
> +}
>
> +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 e820 scan (more than 4 memmap= 
> args)!\n");
> @@ -577,16 +590,7 @@ static unsigned long find_random_phys_addr(unsigned long 
> minimum,
> /* Make sure minimum is aligned. */
> minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
>
> -   /* Verify potential e820 positions, appending to slots list. */
> -   for (i = 0; i < boot_params->e820_entries; i++) {
> -   process_e820_entry(_params->e820_table[i], minimum,
> -  image_size);
> -   if (slot_area_index == MAX_SLOT_AREA) {
> -   debug_putstr("Aborted e820 scan (slot_areas 
> full)!\n");
> -   break;
> -   }
> -   }
> -
> +   process_e820_entries(minimum, image_size);
> return slots_fetch_random();
>  }
>
> --
> 2.5.5
>



-- 
Kees Cook
Pixel Security

[PATCH v4 1/4] x86/boot/KASLR: Wrap e820 entries walking code into new function process_e820_entries()

[Baoquan He]

The original function process_e820_entry() only takes care of each
e820 entry passed.

And move the E820_TYPE_RAM checking logic into process_e820_entries().

And remove the redundent local variable 'addr' definition in
find_random_phys_addr().

Signed-off-by: Baoquan He 
---
 arch/x86/boot/compressed/kaslr.c | 38 +-
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 91f27ab970ef..1485f48aeda1 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -488,10 +488,6 @@ static void process_e820_entry(struct boot_e820_entry 
*entry,
unsigned long start_orig, end;
struct boot_e820_entry cur_entry;
 
-   /* Skip non-RAM entries. */
-   if (entry->type != E820_TYPE_RAM)
-   return;
-
/* On 32-bit, ignore entries entirely above our maximum. */
if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
return;
@@ -562,12 +558,29 @@ static void process_e820_entry(struct boot_e820_entry 
*entry,
}
 }
 
-static unsigned long find_random_phys_addr(unsigned long minimum,
-  unsigned long image_size)
+static void process_e820_entries(unsigned long minimum,
+unsigned long image_size)
 {
int i;
-   unsigned long addr;
+   struct boot_e820_entry *entry;
+
+   /* Verify potential e820 positions, appending to slots list. */
+   for (i = 0; i < boot_params->e820_entries; i++) {
+   entry = _params->e820_table[i];
+   /* Skip non-RAM entries. */
+   if (entry->type != E820_TYPE_RAM)
+   continue;
+   process_e820_entry(entry, minimum, image_size);
+   if (slot_area_index == MAX_SLOT_AREA) {
+   debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+   break;
+   }
+   }
+}
 
+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 e820 scan (more than 4 memmap= args)!\n");
@@ -577,16 +590,7 @@ static unsigned long find_random_phys_addr(unsigned long 
minimum,
/* Make sure minimum is aligned. */
minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
 
-   /* Verify potential e820 positions, appending to slots list. */
-   for (i = 0; i < boot_params->e820_entries; i++) {
-   process_e820_entry(_params->e820_table[i], minimum,
-  image_size);
-   if (slot_area_index == MAX_SLOT_AREA) {
-   debug_putstr("Aborted e820 scan (slot_areas full)!\n");
-   break;
-   }
-   }
-
+   process_e820_entries(minimum, image_size);
return slots_fetch_random();
 }
 
-- 
2.5.5

[PATCH v4 1/4] x86/boot/KASLR: Wrap e820 entries walking code into new function process_e820_entries()

[Baoquan He]

The original function process_e820_entry() only takes care of each
e820 entry passed.

And move the E820_TYPE_RAM checking logic into process_e820_entries().

And remove the redundent local variable 'addr' definition in
find_random_phys_addr().

Signed-off-by: Baoquan He 
---
 arch/x86/boot/compressed/kaslr.c | 38 +-
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 91f27ab970ef..1485f48aeda1 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -488,10 +488,6 @@ static void process_e820_entry(struct boot_e820_entry 
*entry,
unsigned long start_orig, end;
struct boot_e820_entry cur_entry;
 
-   /* Skip non-RAM entries. */
-   if (entry->type != E820_TYPE_RAM)
-   return;
-
/* On 32-bit, ignore entries entirely above our maximum. */
if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
return;
@@ -562,12 +558,29 @@ static void process_e820_entry(struct boot_e820_entry 
*entry,
}
 }
 
-static unsigned long find_random_phys_addr(unsigned long minimum,
-  unsigned long image_size)
+static void process_e820_entries(unsigned long minimum,
+unsigned long image_size)
 {
int i;
-   unsigned long addr;
+   struct boot_e820_entry *entry;
+
+   /* Verify potential e820 positions, appending to slots list. */
+   for (i = 0; i < boot_params->e820_entries; i++) {
+   entry = _params->e820_table[i];
+   /* Skip non-RAM entries. */
+   if (entry->type != E820_TYPE_RAM)
+   continue;
+   process_e820_entry(entry, minimum, image_size);
+   if (slot_area_index == MAX_SLOT_AREA) {
+   debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+   break;
+   }
+   }
+}
 
+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 e820 scan (more than 4 memmap= args)!\n");
@@ -577,16 +590,7 @@ static unsigned long find_random_phys_addr(unsigned long 
minimum,
/* Make sure minimum is aligned. */
minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
 
-   /* Verify potential e820 positions, appending to slots list. */
-   for (i = 0; i < boot_params->e820_entries; i++) {
-   process_e820_entry(_params->e820_table[i], minimum,
-  image_size);
-   if (slot_area_index == MAX_SLOT_AREA) {
-   debug_putstr("Aborted e820 scan (slot_areas full)!\n");
-   break;
-   }
-   }
-
+   process_e820_entries(minimum, image_size);
return slots_fetch_random();
 }
 
-- 
2.5.5