x86/boot: Fix latent memory corruption with early_boot_opts_t

[Xen-devel] [PATCH] x86/boot: Fix latent memory corruption with early_boot_opts_t

Posted by Andrew Cooper 4 years, 11 months ago

c/s ebb26b509f "xen/x86: make VGA support selectable" added an #ifdef
CONFIG_VIDEO into the middle the backing space for early_boot_opts_t,
but didn't adjust the structure definition in cmdline.c

This only functions correctly because the affected fields are at the end
of the structure, and cmdline.c doesn't write to them in this case.

To retain the slimming effect of compiling out CONFIG_VIDEO, adjust
cmdline.c with enough #ifdef-ary to make C's idea of the structure match
the declaration in asm.  This requires adding __maybe_unused annotations
to two helper functions.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: David Woodhouse <dwmw2@infradead.org>

This needs backporting to Xen 4.11

Discovered while reviewing David's "Clean up x86_64 boot code" series.
---
 xen/arch/x86/boot/cmdline.c | 14 ++++++++++----
 xen/arch/x86/boot/defs.h    |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c
index 51b0659..fc11c6d 100644
--- a/xen/arch/x86/boot/cmdline.c
+++ b/xen/arch/x86/boot/cmdline.c
@@ -40,10 +40,12 @@ typedef struct __packed {
     u8 opt_edd;
     u8 opt_edid;
     u8 padding;
+#ifdef CONFIG_VIDEO
     u16 boot_vid_mode;
     u16 vesa_width;
     u16 vesa_height;
     u16 vesa_depth;
+#endif
 } early_boot_opts_t;
 
 /*
@@ -127,7 +129,8 @@ static size_t strcspn(const char *s, const char *reject)
     return count;
 }
 
-static unsigned int strtoui(const char *s, const char *stop, const char **next)
+static unsigned int __maybe_unused strtoui(
+    const char *s, const char *stop, const char **next)
 {
     char base = 10, l;
     unsigned long long res = 0;
@@ -176,7 +179,7 @@ static int strmaxcmp(const char *cs, const char *ct, const char *_delim_chars)
     return strncmp(cs, ct, max(strcspn(cs, _delim_chars), strlen(ct)));
 }
 
-static int strsubcmp(const char *cs, const char *ct)
+static int __maybe_unused strsubcmp(const char *cs, const char *ct)
 {
     return strncmp(cs, ct, strlen(ct));
 }
@@ -241,6 +244,7 @@ static u8 edid_parse(const char *cmdline)
     return !strmaxcmp(c, "no", delim_chars);
 }
 
+#ifdef CONFIG_VIDEO
 static u16 rows2vmode(unsigned int rows)
 {
     switch ( rows )
@@ -328,6 +332,7 @@ static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
         ebo->boot_vid_mode = tmp;
     }
 }
+#endif
 
 void __stdcall cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo)
 {
@@ -338,6 +343,7 @@ void __stdcall cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo)
     ebo->opt_edd = edd_parse(cmdline);
     ebo->opt_edid = edid_parse(cmdline);
 
-    if ( IS_ENABLED(CONFIG_VIDEO) )
-        vga_parse(cmdline, ebo);
+#ifdef CONFIG_VIDEO
+    vga_parse(cmdline, ebo);
+#endif
 }
diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
index 05921a6..f57ad53 100644
--- a/xen/arch/x86/boot/defs.h
+++ b/xen/arch/x86/boot/defs.h
@@ -23,6 +23,7 @@
 #include "../../../include/xen/stdbool.h"
 
 #define __packed	__attribute__((__packed__))
+#define __maybe_unused	__attribute__((__unused__))
 #define __stdcall	__attribute__((__stdcall__))
 
 #define NULL		((void *)0)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/boot: Fix latent memory corruption with early_boot_opts_t

Posted by Jan Beulich 4 years, 11 months ago

>>> On 02.05.19 at 13:52, <andrew.cooper3@citrix.com> wrote:
> c/s ebb26b509f "xen/x86: make VGA support selectable" added an #ifdef
> CONFIG_VIDEO into the middle the backing space for early_boot_opts_t,
> but didn't adjust the structure definition in cmdline.c
> 
> This only functions correctly because the affected fields are at the end
> of the structure, and cmdline.c doesn't write to them in this case.
> 
> To retain the slimming effect of compiling out CONFIG_VIDEO, adjust
> cmdline.c with enough #ifdef-ary to make C's idea of the structure match
> the declaration in asm.  This requires adding __maybe_unused annotations
> to two helper functions.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a remark and a question:

> --- a/xen/arch/x86/boot/cmdline.c
> +++ b/xen/arch/x86/boot/cmdline.c
> @@ -40,10 +40,12 @@ typedef struct __packed {
>      u8 opt_edd;
>      u8 opt_edid;
>      u8 padding;
> +#ifdef CONFIG_VIDEO
>      u16 boot_vid_mode;
>      u16 vesa_width;
>      u16 vesa_height;
>      u16 vesa_depth;
> +#endif

Since apparently the "Keep in sync" comment in trampoline.S
wasn't sufficient, and since - with what said commit did - the
comment now looks unrelated to these data items (for there
being a blank line in between now) perhaps this should be
accompanied by both a START and END marker?

And perhaps the comment next to vesa_size should also
get corrected to say "width x height x depth".

My R-b stands if you decide to fold these in.

> --- a/xen/arch/x86/boot/defs.h
> +++ b/xen/arch/x86/boot/defs.h
> @@ -23,6 +23,7 @@
>  #include "../../../include/xen/stdbool.h"
>  
>  #define __packed	__attribute__((__packed__))
> +#define __maybe_unused	__attribute__((__unused__))
>  #define __stdcall	__attribute__((__stdcall__))

Purely out of curiosity (I don't really care about the ordering
here as long as the set doesn't meaningfully grow): Based on
what did you decide this best goes between the two existing
ones?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/boot: Fix latent memory corruption with early_boot_opts_t

Posted by Andrew Cooper 4 years, 11 months ago

On 02/05/2019 13:44, Jan Beulich wrote:
>>>> On 02.05.19 at 13:52, <andrew.cooper3@citrix.com> wrote:
>> c/s ebb26b509f "xen/x86: make VGA support selectable" added an #ifdef
>> CONFIG_VIDEO into the middle the backing space for early_boot_opts_t,
>> but didn't adjust the structure definition in cmdline.c
>>
>> This only functions correctly because the affected fields are at the end
>> of the structure, and cmdline.c doesn't write to them in this case.
>>
>> To retain the slimming effect of compiling out CONFIG_VIDEO, adjust
>> cmdline.c with enough #ifdef-ary to make C's idea of the structure match
>> the declaration in asm.  This requires adding __maybe_unused annotations
>> to two helper functions.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with a remark and a question:
>
>> --- a/xen/arch/x86/boot/cmdline.c
>> +++ b/xen/arch/x86/boot/cmdline.c
>> @@ -40,10 +40,12 @@ typedef struct __packed {
>>      u8 opt_edd;
>>      u8 opt_edid;
>>      u8 padding;
>> +#ifdef CONFIG_VIDEO
>>      u16 boot_vid_mode;
>>      u16 vesa_width;
>>      u16 vesa_height;
>>      u16 vesa_depth;
>> +#endif
> Since apparently the "Keep in sync" comment in trampoline.S
> wasn't sufficient, and since - with what said commit did - the
> comment now looks unrelated to these data items (for there
> being a blank line in between now) perhaps this should be
> accompanied by both a START and END marker?

I've got a followup patch which cleans up the ASM, but I don't want to
interfere with the work that David is currently preparing.

> And perhaps the comment next to vesa_size should also
> get corrected to say "width x height x depth".

I had already addressed this, and the fact that boot_vid_mode has never
needed to be global (ever since its introduction).

>
>> --- a/xen/arch/x86/boot/defs.h
>> +++ b/xen/arch/x86/boot/defs.h
>> @@ -23,6 +23,7 @@
>>  #include "../../../include/xen/stdbool.h"
>>  
>>  #define __packed	__attribute__((__packed__))
>> +#define __maybe_unused	__attribute__((__unused__))
>>  #define __stdcall	__attribute__((__stdcall__))
> Purely out of curiosity (I don't really care about the ordering
> here as long as the set doesn't meaningfully grow): Based on
> what did you decide this best goes between the two existing
> ones?

I forgot to sort the lines.  Fixed.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel