[PATCH v2] EFI: make for_each_efi_memory_desc_in_map() cope with running on Xen

[PATCH v2] EFI: make for_each_efi_memory_desc_in_map() cope with running on Xen

[Jan Beulich]

While commit 55f1ea15216 ("efi: Fix for_each_efi_memory_desc_in_map()
for empty memmaps") made an attempt to deal with empty memory maps, it
didn't address the case where the desc_size field never gets set, as is
apparently the case when running under Xen.

Reported-by: <lists-Ilq5uHa1fWNWk0Htik3J/w@public.gmane.org>
Cc: Vitaly Kuznetsov <vkuznets-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
Tested-by: <lists-Ilq5uHa1fWNWk0Htik3J/w@public.gmane.org>
---
v2: Add comment.
---
 include/linux/efi.h |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--- 4.8-rc2/include/linux/efi.h
+++ 4.8-rc2-EFI-memdesc-iterator-Xen/include/linux/efi.h
@@ -946,7 +946,11 @@ extern int efi_memattr_apply_permissions
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc_in_map(m, md)				   \
 	for ((md) = (m)->map;						   \
-	     ((void *)(md) + (m)->desc_size) <= (m)->map_end;		   \
+	     /*								   \
+	      * Leverage wrapping of the calculation to avoid an infinite  \
+	      * loop when all three values are zero.			   \
+	      */							   \
+	     ((void *)(md) + (m)->desc_size - 1) < (m)->map_end;	   \
 	     (md) = (void *)(md) + (m)->desc_size)
 
 /**

Re: [PATCH v2] EFI: make for_each_efi_memory_desc_in_map() cope with running on Xen

[Matt Fleming]

On Mon, 15 Aug, at 09:05:45AM, Jan Beulich wrote:
> While commit 55f1ea15216 ("efi: Fix for_each_efi_memory_desc_in_map()
> for empty memmaps") made an attempt to deal with empty memory maps, it
> didn't address the case where the desc_size field never gets set, as is
> apparently the case when running under Xen.
> 
> Reported-by: <lists-Ilq5uHa1fWNWk0Htik3J/w@public.gmane.org>
> Cc: Vitaly Kuznetsov <vkuznets-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
> Tested-by: <lists-Ilq5uHa1fWNWk0Htik3J/w@public.gmane.org>
> ---
> v2: Add comment.
> ---
>  include/linux/efi.h |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> --- 4.8-rc2/include/linux/efi.h
> +++ 4.8-rc2-EFI-memdesc-iterator-Xen/include/linux/efi.h
> @@ -946,7 +946,11 @@ extern int efi_memattr_apply_permissions
>  /* Iterate through an efi_memory_map */
>  #define for_each_efi_memory_desc_in_map(m, md)				   \
>  	for ((md) = (m)->map;						   \
> -	     ((void *)(md) + (m)->desc_size) <= (m)->map_end;		   \
> +	     /*								   \
> +	      * Leverage wrapping of the calculation to avoid an infinite  \
> +	      * loop when all three values are zero.			   \
> +	      */							   \
> +	     ((void *)(md) + (m)->desc_size - 1) < (m)->map_end;	   \
>  	     (md) = (void *)(md) + (m)->desc_size)
>  

Thanks Jan, applied to the 'urgent' EFI queue and tagged for stable.
I'll send it out this week.

Re: [PATCH v2] EFI: make for_each_efi_memory_desc_in_map() cope with running on Xen

[Matt Fleming]

On Tue, 16 Aug, at 12:49:17PM, Matt Fleming wrote:
> On Mon, 15 Aug, at 09:05:45AM, Jan Beulich wrote:
> > While commit 55f1ea15216 ("efi: Fix for_each_efi_memory_desc_in_map()
> > for empty memmaps") made an attempt to deal with empty memory maps, it
> > didn't address the case where the desc_size field never gets set, as is
> > apparently the case when running under Xen.
> > 
> > Reported-by: <lists-Ilq5uHa1fWNWk0Htik3J/w@public.gmane.org>
> > Cc: Vitaly Kuznetsov <vkuznets-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> > Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
> > Tested-by: <lists-Ilq5uHa1fWNWk0Htik3J/w@public.gmane.org>
> > ---
> > v2: Add comment.
> > ---
> >  include/linux/efi.h |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > --- 4.8-rc2/include/linux/efi.h
> > +++ 4.8-rc2-EFI-memdesc-iterator-Xen/include/linux/efi.h
> > @@ -946,7 +946,11 @@ extern int efi_memattr_apply_permissions
> >  /* Iterate through an efi_memory_map */
> >  #define for_each_efi_memory_desc_in_map(m, md)				   \
> >  	for ((md) = (m)->map;						   \
> > -	     ((void *)(md) + (m)->desc_size) <= (m)->map_end;		   \
> > +	     /*								   \
> > +	      * Leverage wrapping of the calculation to avoid an infinite  \
> > +	      * loop when all three values are zero.			   \
> > +	      */							   \
> > +	     ((void *)(md) + (m)->desc_size - 1) < (m)->map_end;	   \
> >  	     (md) = (void *)(md) + (m)->desc_size)
> >  
> 
> Thanks Jan, applied to the 'urgent' EFI queue and tagged for stable.
> I'll send it out this week.

Jiri caught me on IRC and repeated his concern that this patch relies
on undefined behaviour per the C language standard.

I really don't want to get into a discussion of compiler
implementations, so instead can we check whether the 'md' is NULL and
skip the loop if so? e.g. would something like this work?

---

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f5a58225385..23cd3ced8c1a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -946,7 +946,7 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc_in_map(m, md)				   \
 	for ((md) = (m)->map;						   \
-	     ((void *)(md) + (m)->desc_size) <= (m)->map_end;		   \
+	     (md) && ((void *)(md) + (m)->desc_size) <= (m)->map_end;	   \
 	     (md) = (void *)(md) + (m)->desc_size)
 
 /**

Re: [PATCH v2] EFI: make for_each_efi_memory_desc_in_map() cope with running on Xen

[Jan Beulich]

>>> On 19.08.16 at 12:47, <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Tue, 16 Aug, at 12:49:17PM, Matt Fleming wrote:
>> On Mon, 15 Aug, at 09:05:45AM, Jan Beulich wrote:
>> > --- 4.8-rc2/include/linux/efi.h
>> > +++ 4.8-rc2-EFI-memdesc-iterator-Xen/include/linux/efi.h
>> > @@ -946,7 +946,11 @@ extern int efi_memattr_apply_permissions
>> >  /* Iterate through an efi_memory_map */
>> >  #define for_each_efi_memory_desc_in_map(m, md)				   \
>> >  	for ((md) = (m)->map;						   \
>> > -	     ((void *)(md) + (m)->desc_size) <= (m)->map_end;		   \
>> > +	     /*								   \
>> > +	      * Leverage wrapping of the calculation to avoid an infinite  \
>> > +	      * loop when all three values are zero.			   \
>> > +	      */							   \
>> > +	     ((void *)(md) + (m)->desc_size - 1) < (m)->map_end;	   \
>> >  	     (md) = (void *)(md) + (m)->desc_size)
>> >  
>> 
>> Thanks Jan, applied to the 'urgent' EFI queue and tagged for stable.
>> I'll send it out this week.
> 
> Jiri caught me on IRC and repeated his concern that this patch relies
> on undefined behaviour per the C language standard.
> 
> I really don't want to get into a discussion of compiler
> implementations, so instead can we check whether the 'md' is NULL and
> skip the loop if so? e.g. would something like this work?

I think so, yes, but I've not seen the issue myself, so I can't verify
that's the case.

Jan

> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -946,7 +946,7 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm,
>  /* Iterate through an efi_memory_map */
>  #define for_each_efi_memory_desc_in_map(m, md)				   \
>  	for ((md) = (m)->map;						   \
> -	     ((void *)(md) + (m)->desc_size) <= (m)->map_end;		   \
> +	     (md) && ((void *)(md) + (m)->desc_size) <= (m)->map_end;	   \
>  	     (md) = (void *)(md) + (m)->desc_size)
>  
>  /**