Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1680742 > unrolled thread
| Started by | Baoquan He <bhe@redhat.com> |
|---|---|
| First post | 2017-07-04 10:10 +0200 |
| Last post | 2017-07-06 03:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH v2 1/2] x86/boot/KASLR: Adapt process_e820_entry for any type of memory entry Baoquan He <bhe@redhat.com> - 2017-07-04 10:10 +0200
Re: [PATCH v2 1/2] x86/boot/KASLR: Adapt process_e820_entry for any type of memory entry Kees Cook <keescook@chromium.org> - 2017-07-06 00:10 +0200
Re: [PATCH v2 1/2] x86/boot/KASLR: Adapt process_e820_entry for any type of memory entry Baoquan He <bhe@redhat.com> - 2017-07-06 03:30 +0200
| From | Baoquan He <bhe@redhat.com> |
|---|---|
| Date | 2017-07-04 10:10 +0200 |
| Subject | [PATCH v2 1/2] x86/boot/KASLR: Adapt process_e820_entry for any type of memory entry |
| Message-ID | <tZr4g-5Hh-33@gated-at.bofh.it> |
Now function process_e820_entry is only used to process e820 memory
entries. Adapt it for any type of memory entry, not just for e820.
Later we will use it to process efi mirror regions.
So rename the old process_e820_entry to process_mem_region, and
extract and wrap the e820 specific processing code into process_e820_entry.
Signed-off-by: Baoquan He <bhe@redhat.com>
---
arch/x86/boot/compressed/kaslr.c | 60 ++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 27 deletions(-)
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 91f27ab970ef..85c360eec4a6 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -479,35 +479,31 @@ static unsigned long slots_fetch_random(void)
return 0;
}
-static void process_e820_entry(struct boot_e820_entry *entry,
+static void process_mem_region(struct mem_vector *entry,
unsigned long minimum,
unsigned long image_size)
{
struct mem_vector region, overlap;
struct slot_area slot_area;
unsigned long start_orig, end;
- struct boot_e820_entry cur_entry;
-
- /* Skip non-RAM entries. */
- if (entry->type != E820_TYPE_RAM)
- return;
+ struct mem_vector cur_entry;
/* On 32-bit, ignore entries entirely above our maximum. */
- if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
+ if (IS_ENABLED(CONFIG_X86_32) && entry->start >= KERNEL_IMAGE_SIZE)
return;
/* Ignore entries entirely below our minimum. */
- if (entry->addr + entry->size < minimum)
+ if (entry->start + entry->size < minimum)
return;
/* Ignore entries above memory limit */
- end = min(entry->size + entry->addr, mem_limit);
- if (entry->addr >= end)
+ end = min(entry->size + entry->start, mem_limit);
+ if (entry->start >= end)
return;
- cur_entry.addr = entry->addr;
- cur_entry.size = end - entry->addr;
+ cur_entry.start = entry->start;
+ cur_entry.size = end - entry->start;
- region.start = cur_entry.addr;
+ region.start = cur_entry.start;
region.size = cur_entry.size;
/* Give up if slot area array is full. */
@@ -522,7 +518,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
/* Did we raise the address above this e820 region? */
- if (region.start > cur_entry.addr + cur_entry.size)
+ if (region.start > cur_entry.start + cur_entry.size)
return;
/* Reduce size by any delta from the original address. */
@@ -562,12 +558,31 @@ 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_entry(unsigned long minimum, unsigned long image_size)
{
int i;
- unsigned long addr;
+ struct mem_vector region;
+ struct boot_e820_entry *entry;
+
+ /* Verify potential e820 positions, appending to slots list. */
+ for (i = 0; i < boot_params->e820_entries; i++) {
+ entry = &boot_params->e820_table[i];
+ /* Skip non-RAM entries. */
+ if (entry->type != E820_TYPE_RAM)
+ continue;
+ region.start = entry->addr;
+ region.size = entry->size;
+ process_mem_region(®ion, 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 +592,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(&boot_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_entry(minimum, image_size);
return slots_fetch_random();
}
--
2.5.5
[toc] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-07-06 00:10 +0200 |
| Subject | Re: [PATCH v2 1/2] x86/boot/KASLR: Adapt process_e820_entry for any type of memory entry |
| Message-ID | <u00EG-3NZ-17@gated-at.bofh.it> |
| In reply to | #1680742 |
On Tue, Jul 4, 2017 at 1:04 AM, Baoquan He <bhe@redhat.com> wrote:
> Now function process_e820_entry is only used to process e820 memory
> entries. Adapt it for any type of memory entry, not just for e820.
> Later we will use it to process efi mirror regions.
>
> So rename the old process_e820_entry to process_mem_region, and
> extract and wrap the e820 specific processing code into process_e820_entry.
This mixes changing the structure internals (.addr vs .start) with
changing the processing logic (adding a wrapper). I would prefer this
was done in several stages to make review easier:
1) lift e820 walking into a new function process_e820_entries(), and
move TYPE_RAM logic there.
2) switch process_e820_entry() from struct boot_e820_entry to struct mem_vector.
3) rename (and adjust comments!) of process_e820_entry() into
process_mem_region().
-Kees
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> arch/x86/boot/compressed/kaslr.c | 60 ++++++++++++++++++++++------------------
> 1 file changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 91f27ab970ef..85c360eec4a6 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -479,35 +479,31 @@ static unsigned long slots_fetch_random(void)
> return 0;
> }
>
> -static void process_e820_entry(struct boot_e820_entry *entry,
> +static void process_mem_region(struct mem_vector *entry,
> unsigned long minimum,
> unsigned long image_size)
> {
> struct mem_vector region, overlap;
> struct slot_area slot_area;
> unsigned long start_orig, end;
> - struct boot_e820_entry cur_entry;
> -
> - /* Skip non-RAM entries. */
> - if (entry->type != E820_TYPE_RAM)
> - return;
> + struct mem_vector cur_entry;
>
> /* On 32-bit, ignore entries entirely above our maximum. */
> - if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
> + if (IS_ENABLED(CONFIG_X86_32) && entry->start >= KERNEL_IMAGE_SIZE)
> return;
>
> /* Ignore entries entirely below our minimum. */
> - if (entry->addr + entry->size < minimum)
> + if (entry->start + entry->size < minimum)
> return;
>
> /* Ignore entries above memory limit */
> - end = min(entry->size + entry->addr, mem_limit);
> - if (entry->addr >= end)
> + end = min(entry->size + entry->start, mem_limit);
> + if (entry->start >= end)
> return;
> - cur_entry.addr = entry->addr;
> - cur_entry.size = end - entry->addr;
> + cur_entry.start = entry->start;
> + cur_entry.size = end - entry->start;
>
> - region.start = cur_entry.addr;
> + region.start = cur_entry.start;
> region.size = cur_entry.size;
>
> /* Give up if slot area array is full. */
> @@ -522,7 +518,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
>
> /* Did we raise the address above this e820 region? */
> - if (region.start > cur_entry.addr + cur_entry.size)
> + if (region.start > cur_entry.start + cur_entry.size)
> return;
>
> /* Reduce size by any delta from the original address. */
> @@ -562,12 +558,31 @@ 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_entry(unsigned long minimum, unsigned long image_size)
> {
> int i;
> - unsigned long addr;
> + struct mem_vector region;
> + struct boot_e820_entry *entry;
> +
> + /* Verify potential e820 positions, appending to slots list. */
> + for (i = 0; i < boot_params->e820_entries; i++) {
> + entry = &boot_params->e820_table[i];
> + /* Skip non-RAM entries. */
> + if (entry->type != E820_TYPE_RAM)
> + continue;
> + region.start = entry->addr;
> + region.size = entry->size;
> + process_mem_region(®ion, 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 +592,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(&boot_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_entry(minimum, image_size);
> return slots_fetch_random();
> }
>
> --
> 2.5.5
>
--
Kees Cook
Pixel Security
[toc] | [prev] | [next] | [standalone]
| From | Baoquan He <bhe@redhat.com> |
|---|---|
| Date | 2017-07-06 03:30 +0200 |
| Subject | Re: [PATCH v2 1/2] x86/boot/KASLR: Adapt process_e820_entry for any type of memory entry |
| Message-ID | <u03Md-5NV-3@gated-at.bofh.it> |
| In reply to | #1681903 |
On 07/05/17 at 03:06pm, Kees Cook wrote:
> On Tue, Jul 4, 2017 at 1:04 AM, Baoquan He <bhe@redhat.com> wrote:
> > Now function process_e820_entry is only used to process e820 memory
> > entries. Adapt it for any type of memory entry, not just for e820.
> > Later we will use it to process efi mirror regions.
> >
> > So rename the old process_e820_entry to process_mem_region, and
> > extract and wrap the e820 specific processing code into process_e820_entry.
>
> This mixes changing the structure internals (.addr vs .start) with
> changing the processing logic (adding a wrapper). I would prefer this
> was done in several stages to make review easier:
OK, will do following below steps. Thanks!
>
> 1) lift e820 walking into a new function process_e820_entries(), and
> move TYPE_RAM logic there.
>
> 2) switch process_e820_entry() from struct boot_e820_entry to struct mem_vector.
>
> 3) rename (and adjust comments!) of process_e820_entry() into
> process_mem_region().
>
> -Kees
>
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> > arch/x86/boot/compressed/kaslr.c | 60 ++++++++++++++++++++++------------------
> > 1 file changed, 33 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> > index 91f27ab970ef..85c360eec4a6 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -479,35 +479,31 @@ static unsigned long slots_fetch_random(void)
> > return 0;
> > }
> >
> > -static void process_e820_entry(struct boot_e820_entry *entry,
> > +static void process_mem_region(struct mem_vector *entry,
> > unsigned long minimum,
> > unsigned long image_size)
> > {
> > struct mem_vector region, overlap;
> > struct slot_area slot_area;
> > unsigned long start_orig, end;
> > - struct boot_e820_entry cur_entry;
> > -
> > - /* Skip non-RAM entries. */
> > - if (entry->type != E820_TYPE_RAM)
> > - return;
> > + struct mem_vector cur_entry;
> >
> > /* On 32-bit, ignore entries entirely above our maximum. */
> > - if (IS_ENABLED(CONFIG_X86_32) && entry->addr >= KERNEL_IMAGE_SIZE)
> > + if (IS_ENABLED(CONFIG_X86_32) && entry->start >= KERNEL_IMAGE_SIZE)
> > return;
> >
> > /* Ignore entries entirely below our minimum. */
> > - if (entry->addr + entry->size < minimum)
> > + if (entry->start + entry->size < minimum)
> > return;
> >
> > /* Ignore entries above memory limit */
> > - end = min(entry->size + entry->addr, mem_limit);
> > - if (entry->addr >= end)
> > + end = min(entry->size + entry->start, mem_limit);
> > + if (entry->start >= end)
> > return;
> > - cur_entry.addr = entry->addr;
> > - cur_entry.size = end - entry->addr;
> > + cur_entry.start = entry->start;
> > + cur_entry.size = end - entry->start;
> >
> > - region.start = cur_entry.addr;
> > + region.start = cur_entry.start;
> > region.size = cur_entry.size;
> >
> > /* Give up if slot area array is full. */
> > @@ -522,7 +518,7 @@ static void process_e820_entry(struct boot_e820_entry *entry,
> > region.start = ALIGN(region.start, CONFIG_PHYSICAL_ALIGN);
> >
> > /* Did we raise the address above this e820 region? */
> > - if (region.start > cur_entry.addr + cur_entry.size)
> > + if (region.start > cur_entry.start + cur_entry.size)
> > return;
> >
> > /* Reduce size by any delta from the original address. */
> > @@ -562,12 +558,31 @@ 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_entry(unsigned long minimum, unsigned long image_size)
> > {
> > int i;
> > - unsigned long addr;
> > + struct mem_vector region;
> > + struct boot_e820_entry *entry;
> > +
> > + /* Verify potential e820 positions, appending to slots list. */
> > + for (i = 0; i < boot_params->e820_entries; i++) {
> > + entry = &boot_params->e820_table[i];
> > + /* Skip non-RAM entries. */
> > + if (entry->type != E820_TYPE_RAM)
> > + continue;
> > + region.start = entry->addr;
> > + region.size = entry->size;
> > + process_mem_region(®ion, 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 +592,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(&boot_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_entry(minimum, image_size);
> > return slots_fetch_random();
> > }
> >
> > --
> > 2.5.5
> >
>
>
>
> --
> Kees Cook
> Pixel Security
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web