Fwd: Re: [PATCH v2 6/6] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

2015-11-09 Thread Vladimir 'φ-coder/phcoder' Serbinenko 



 Forwarded Message 
Subject:Re: [PATCH v2 6/6] multiboot2: Do not pass memory maps to
image if EFI boot services are enabled
Date:   Mon, 9 Nov 2015 20:07:23 +0100
From:   Vladimir 'phcoder' Serbinenko 
To: Daniel Kiper 



This one is good to go. But the main reason for not providing the map is
because it will likely be invalid. We do few allocations after filling
the map I.a. git relocator. Usually we don't care as we would already
finish boot services but if we keep them, it's easier not to provide
them, at least until somebody needs then. Please adjust the description
and add or to the comment

Le 20 juil. 2015 4:37 PM, "Daniel Kiper" > a écrit :

Do not pass memory maps to image if it asked for EFI boot services.
Maps are
usually invalid in that case and they can confuse potential user.
Image should
get memory map itself just before ExitBootServices() call.

Signed-off-by: Daniel Kiper >
---
 grub-core/loader/multiboot_mbi2.c |   71
++---
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c
b/grub-core/loader/multiboot_mbi2.c
index 7ac64ec..26e955c 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -431,7 +431,7 @@ static grub_size_t
 grub_multiboot_get_mbi_size (void)
 {
 #ifdef GRUB_MACHINE_EFI
-  if (!efi_mmap_size)
+  if (!keep_bs && !efi_mmap_size)
 find_efi_mmap_size ();
 #endif
   return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
@@ -805,12 +805,13 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
   }
   }

-  {
-struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *)
ptrorig;
-grub_fill_multiboot_mmap (tag);
-ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-  / sizeof (grub_properly_aligned_t);
-  }
+  if (!keep_bs)
+{
+  struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap
*) ptrorig;
+  grub_fill_multiboot_mmap (tag);
+  ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+   / sizeof (grub_properly_aligned_t);
+}

   {
 struct multiboot_tag_elf_sections *tag
@@ -826,18 +827,19 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
   / sizeof (grub_properly_aligned_t);
   }

-  {
-struct multiboot_tag_basic_meminfo *tag
-  = (struct multiboot_tag_basic_meminfo *) ptrorig;
-tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
-tag->size = sizeof (struct multiboot_tag_basic_meminfo);
+  if (!keep_bs)
+{
+  struct multiboot_tag_basic_meminfo *tag
+   = (struct multiboot_tag_basic_meminfo *) ptrorig;
+  tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
+  tag->size = sizeof (struct multiboot_tag_basic_meminfo);

-/* Convert from bytes to kilobytes.  */
-tag->mem_lower = grub_mmap_get_lower () / 1024;
-tag->mem_upper = grub_mmap_get_upper () / 1024;
-ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-   / sizeof (grub_properly_aligned_t);
-  }
+  /* Convert from bytes to kilobytes.  */
+  tag->mem_lower = grub_mmap_get_lower () / 1024;
+  tag->mem_upper = grub_mmap_get_upper () / 1024;
+  ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+   / sizeof (grub_properly_aligned_t);
+}

   {
 struct grub_net_network_level_interface *net;
@@ -936,27 +938,24 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
 grub_efi_uintn_t efi_desc_size;
 grub_efi_uint32_t efi_desc_version;

-tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
-tag->size = sizeof (*tag) + efi_mmap_size;
-
 if (!keep_bs)
-  err = grub_efi_finish_boot_services (_mmap_size,
tag->efi_mmap, NULL,
-  _desc_size,
_desc_version);
-else
   {
-   if (grub_efi_get_memory_map (_mmap_size, (void *)
tag->efi_mmap,
-NULL,
-_desc_size,
_desc_version) <= 0)
- err = grub_error (GRUB_ERR_IO, "couldn't retrieve memory
map");
+   tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
+   tag->size = sizeof (*tag) + efi_mmap_size;
+
+   err = grub_efi_finish_boot_services (_mmap_size,
tag->efi_mmap, NULL,
+_desc_size,
_desc_version);
+
+   if (err)
+ return err;
+
+   tag->descr_size = efi_desc_size;
+   tag->descr_vers = efi_desc_version;
+   tag->size = sizeof

Fwd: Re: [PATCH v2 6/6] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

2015-11-09 Thread Vladimir 'φ-coder/phcoder' Serbinenko 



 Forwarded Message 
Subject:Re: [PATCH v2 6/6] multiboot2: Do not pass memory maps to
image if EFI boot services are enabled
Date:   Mon, 9 Nov 2015 20:08:38 +0100
From:   Vladimir 'phcoder' Serbinenko 
To: Daniel Kiper 



And you also need to adjust loader code to reject images which request
memory tags and bs at the same time

Le 9 nov. 2015 8:07 PM, "Vladimir 'phcoder' Serbinenko"
> a écrit :

This one is good to go. But the main reason for not providing the
map is because it will likely be invalid. We do few allocations
after filling the map I.a. git relocator. Usually we don't care as
we would already finish boot services but if we keep them, it's
easier not to provide them, at least until somebody needs then.
Please adjust the description and add or to the comment

Le 20 juil. 2015 4:37 PM, "Daniel Kiper" > a écrit :

Do not pass memory maps to image if it asked for EFI boot
services. Maps are
usually invalid in that case and they can confuse potential
user. Image should
get memory map itself just before ExitBootServices() call.

Signed-off-by: Daniel Kiper >
---
 grub-core/loader/multiboot_mbi2.c |   71
++---
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c
b/grub-core/loader/multiboot_mbi2.c
index 7ac64ec..26e955c 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -431,7 +431,7 @@ static grub_size_t
 grub_multiboot_get_mbi_size (void)
 {
 #ifdef GRUB_MACHINE_EFI
-  if (!efi_mmap_size)
+  if (!keep_bs && !efi_mmap_size)
 find_efi_mmap_size ();
 #endif
   return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
@@ -805,12 +805,13 @@ grub_multiboot_make_mbi (grub_uint32_t
*target)
   }
   }

-  {
-struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap
*) ptrorig;
-grub_fill_multiboot_mmap (tag);
-ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-  / sizeof (grub_properly_aligned_t);
-  }
+  if (!keep_bs)
+{
+  struct multiboot_tag_mmap *tag = (struct
multiboot_tag_mmap *) ptrorig;
+  grub_fill_multiboot_mmap (tag);
+  ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+   / sizeof (grub_properly_aligned_t);
+}

   {
 struct multiboot_tag_elf_sections *tag
@@ -826,18 +827,19 @@ grub_multiboot_make_mbi (grub_uint32_t
*target)
   / sizeof (grub_properly_aligned_t);
   }

-  {
-struct multiboot_tag_basic_meminfo *tag
-  = (struct multiboot_tag_basic_meminfo *) ptrorig;
-tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
-tag->size = sizeof (struct multiboot_tag_basic_meminfo);
+  if (!keep_bs)
+{
+  struct multiboot_tag_basic_meminfo *tag
+   = (struct multiboot_tag_basic_meminfo *) ptrorig;
+  tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
+  tag->size = sizeof (struct multiboot_tag_basic_meminfo);

-/* Convert from bytes to kilobytes.  */
-tag->mem_lower = grub_mmap_get_lower () / 1024;
-tag->mem_upper = grub_mmap_get_upper () / 1024;
-ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
-   / sizeof (grub_properly_aligned_t);
-  }
+  /* Convert from bytes to kilobytes.  */
+  tag->mem_lower = grub_mmap_get_lower () / 1024;
+  tag->mem_upper = grub_mmap_get_upper () / 1024;
+  ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN)
+   / sizeof (grub_properly_aligned_t);
+}

   {
 struct grub_net_network_level_interface *net;
@@ -936,27 +938,24 @@ grub_multiboot_make_mbi (grub_uint32_t
*target)
 grub_efi_uintn_t efi_desc_size;
 grub_efi_uint32_t efi_desc_version;

-tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
-tag->size = sizeof (*tag) + efi_mmap_size;
-
 if (!keep_bs)
-  err = grub_efi_finish_boot_services (_mmap_size,
tag->efi_mmap, NULL,
-  _desc_size,
_desc_version);
-else
   {
-   if (grub_efi_get_memory_map (_mmap_size, (void *)
tag->efi_mmap,
-

Re: [PATCH v2 6/6] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

2015-08-18 Thread Daniel Kiper 
On Tue, Aug 11, 2015 at 02:59:56PM -0400, Konrad Rzeszutek Wilk wrote:
 On Mon, Jul 20, 2015 at 04:35:54PM +0200, Daniel Kiper wrote:
  Do not pass memory maps to image if it asked for EFI boot services. Maps are
  usually invalid in that case and they can confuse potential user. Image 
  should
  get memory map itself just before ExitBootServices() call.

 Can we point to some commits in Linux or Xen in which these situations
 arose?

 Wait, I think there even was one commit in grub.

 Aha:

 ommit e75fdee420a7ad95e9a465c9699adc2e2e970440
 Author: Vladimir 'phcoder' Serbinenko phco...@gmail.com
 Date:   Tue Mar 26 11:34:56 2013 +0100

 * grub-core/kern/efi/mm.c (grub_efi_finish_boot_services):
 Try terminating EFI services several times due to quirks in some
 implementations.

 Otherwise:

 Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com

Nope, this is a bit different problem. In general GRUB2 assumes that
EFI boot services memory regions are free and specify them as such
in relevant maps. However, this is not true if you jump into loaded
image with EFI boot services enabled. So, we should not expose this
maps to the user when he/she asked for access to EFI boot services.
This is not very big issue for him/her because he/she can get memory
map from EFI just before ExitBootServices() call. This way they will
have latest valid memory map.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 6/6] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

2015-08-11 Thread Konrad Rzeszutek Wilk 
On Mon, Jul 20, 2015 at 04:35:54PM +0200, Daniel Kiper wrote:
 Do not pass memory maps to image if it asked for EFI boot services. Maps are
 usually invalid in that case and they can confuse potential user. Image should
 get memory map itself just before ExitBootServices() call.

Can we point to some commits in Linux or Xen in which these situations
arose? 

Wait, I think there even was one commit in grub.

Aha:

ommit e75fdee420a7ad95e9a465c9699adc2e2e970440
Author: Vladimir 'phcoder' Serbinenko phco...@gmail.com
Date:   Tue Mar 26 11:34:56 2013 +0100

* grub-core/kern/efi/mm.c (grub_efi_finish_boot_services):
Try terminating EFI services several times due to quirks in some
implementations.

Otherwise:

Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com

 
 Signed-off-by: Daniel Kiper daniel.ki...@oracle.com
 ---
  grub-core/loader/multiboot_mbi2.c |   71 
 ++---
  1 file changed, 35 insertions(+), 36 deletions(-)
 
 diff --git a/grub-core/loader/multiboot_mbi2.c 
 b/grub-core/loader/multiboot_mbi2.c
 index 7ac64ec..26e955c 100644
 --- a/grub-core/loader/multiboot_mbi2.c
 +++ b/grub-core/loader/multiboot_mbi2.c
 @@ -431,7 +431,7 @@ static grub_size_t
  grub_multiboot_get_mbi_size (void)
  {
  #ifdef GRUB_MACHINE_EFI
 -  if (!efi_mmap_size)
 +  if (!keep_bs  !efi_mmap_size)
  find_efi_mmap_size ();
  #endif
return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
 @@ -805,12 +805,13 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
}
}
  
 -  {
 -struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
 -grub_fill_multiboot_mmap (tag);
 -ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
 -  / sizeof (grub_properly_aligned_t);
 -  }
 +  if (!keep_bs)
 +{
 +  struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
 +  grub_fill_multiboot_mmap (tag);
 +  ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
 + / sizeof (grub_properly_aligned_t);
 +}
  
{
  struct multiboot_tag_elf_sections *tag
 @@ -826,18 +827,19 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
/ sizeof (grub_properly_aligned_t);
}
  
 -  {
 -struct multiboot_tag_basic_meminfo *tag
 -  = (struct multiboot_tag_basic_meminfo *) ptrorig;
 -tag-type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
 -tag-size = sizeof (struct multiboot_tag_basic_meminfo); 
 +  if (!keep_bs)
 +{
 +  struct multiboot_tag_basic_meminfo *tag
 + = (struct multiboot_tag_basic_meminfo *) ptrorig;
 +  tag-type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
 +  tag-size = sizeof (struct multiboot_tag_basic_meminfo);
  
 -/* Convert from bytes to kilobytes.  */
 -tag-mem_lower = grub_mmap_get_lower () / 1024;
 -tag-mem_upper = grub_mmap_get_upper () / 1024;
 -ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
 -   / sizeof (grub_properly_aligned_t);
 -  }
 +  /* Convert from bytes to kilobytes.  */
 +  tag-mem_lower = grub_mmap_get_lower () / 1024;
 +  tag-mem_upper = grub_mmap_get_upper () / 1024;
 +  ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
 + / sizeof (grub_properly_aligned_t);
 +}
  
{
  struct grub_net_network_level_interface *net;
 @@ -936,27 +938,24 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
  grub_efi_uintn_t efi_desc_size;
  grub_efi_uint32_t efi_desc_version;
  
 -tag-type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
 -tag-size = sizeof (*tag) + efi_mmap_size;
 -
  if (!keep_bs)
 -  err = grub_efi_finish_boot_services (efi_mmap_size, tag-efi_mmap, 
 NULL,
 -efi_desc_size, efi_desc_version);
 -else
{
 - if (grub_efi_get_memory_map (efi_mmap_size, (void *) tag-efi_mmap,
 -  NULL,
 -  efi_desc_size, efi_desc_version) = 0)
 -   err = grub_error (GRUB_ERR_IO, couldn't retrieve memory map);
 + tag-type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
 + tag-size = sizeof (*tag) + efi_mmap_size;
 +
 + err = grub_efi_finish_boot_services (efi_mmap_size, tag-efi_mmap, 
 NULL,
 +  efi_desc_size, efi_desc_version);
 +
 + if (err)
 +   return err;
 +
 + tag-descr_size = efi_desc_size;
 + tag-descr_vers = efi_desc_version;
 + tag-size = sizeof (*tag) + efi_mmap_size;
 +
 + ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
 +   / sizeof (grub_properly_aligned_t);
}
 -if (err)
 -  return err;
 -tag-descr_size = efi_desc_size;
 -tag-descr_vers = efi_desc_version;
 -tag-size = sizeof (*tag) + efi_mmap_size;
 -
 -ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
 -  / sizeof (grub_properly_aligned_t);
}
  
if (keep_bs)
 -- 
 1.7.10.4
 

___
Grub-devel mailing list
Grub-devel@gnu.org

[PATCH v2 6/6] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

2015-07-20 Thread Daniel Kiper 
Do not pass memory maps to image if it asked for EFI boot services. Maps are
usually invalid in that case and they can confuse potential user. Image should
get memory map itself just before ExitBootServices() call.

Signed-off-by: Daniel Kiper daniel.ki...@oracle.com
---
 grub-core/loader/multiboot_mbi2.c |   71 ++---
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c 
b/grub-core/loader/multiboot_mbi2.c
index 7ac64ec..26e955c 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -431,7 +431,7 @@ static grub_size_t
 grub_multiboot_get_mbi_size (void)
 {
 #ifdef GRUB_MACHINE_EFI
-  if (!efi_mmap_size)
+  if (!keep_bs  !efi_mmap_size)
 find_efi_mmap_size ();
 #endif
   return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
@@ -805,12 +805,13 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
   }
   }
 
-  {
-struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
-grub_fill_multiboot_mmap (tag);
-ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
-  / sizeof (grub_properly_aligned_t);
-  }
+  if (!keep_bs)
+{
+  struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig;
+  grub_fill_multiboot_mmap (tag);
+  ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
+   / sizeof (grub_properly_aligned_t);
+}
 
   {
 struct multiboot_tag_elf_sections *tag
@@ -826,18 +827,19 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
   / sizeof (grub_properly_aligned_t);
   }
 
-  {
-struct multiboot_tag_basic_meminfo *tag
-  = (struct multiboot_tag_basic_meminfo *) ptrorig;
-tag-type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
-tag-size = sizeof (struct multiboot_tag_basic_meminfo); 
+  if (!keep_bs)
+{
+  struct multiboot_tag_basic_meminfo *tag
+   = (struct multiboot_tag_basic_meminfo *) ptrorig;
+  tag-type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
+  tag-size = sizeof (struct multiboot_tag_basic_meminfo);
 
-/* Convert from bytes to kilobytes.  */
-tag-mem_lower = grub_mmap_get_lower () / 1024;
-tag-mem_upper = grub_mmap_get_upper () / 1024;
-ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
-   / sizeof (grub_properly_aligned_t);
-  }
+  /* Convert from bytes to kilobytes.  */
+  tag-mem_lower = grub_mmap_get_lower () / 1024;
+  tag-mem_upper = grub_mmap_get_upper () / 1024;
+  ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
+   / sizeof (grub_properly_aligned_t);
+}
 
   {
 struct grub_net_network_level_interface *net;
@@ -936,27 +938,24 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
 grub_efi_uintn_t efi_desc_size;
 grub_efi_uint32_t efi_desc_version;
 
-tag-type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
-tag-size = sizeof (*tag) + efi_mmap_size;
-
 if (!keep_bs)
-  err = grub_efi_finish_boot_services (efi_mmap_size, tag-efi_mmap, NULL,
-  efi_desc_size, efi_desc_version);
-else
   {
-   if (grub_efi_get_memory_map (efi_mmap_size, (void *) tag-efi_mmap,
-NULL,
-efi_desc_size, efi_desc_version) = 0)
- err = grub_error (GRUB_ERR_IO, couldn't retrieve memory map);
+   tag-type = MULTIBOOT_TAG_TYPE_EFI_MMAP;
+   tag-size = sizeof (*tag) + efi_mmap_size;
+
+   err = grub_efi_finish_boot_services (efi_mmap_size, tag-efi_mmap, 
NULL,
+efi_desc_size, efi_desc_version);
+
+   if (err)
+ return err;
+
+   tag-descr_size = efi_desc_size;
+   tag-descr_vers = efi_desc_version;
+   tag-size = sizeof (*tag) + efi_mmap_size;
+
+   ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
+ / sizeof (grub_properly_aligned_t);
   }
-if (err)
-  return err;
-tag-descr_size = efi_desc_size;
-tag-descr_vers = efi_desc_version;
-tag-size = sizeof (*tag) + efi_mmap_size;
-
-ptrorig += ALIGN_UP (tag-size, MULTIBOOT_TAG_ALIGN)
-  / sizeof (grub_properly_aligned_t);
   }
 
   if (keep_bs)
-- 
1.7.10.4


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel