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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.19 at 13:52, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel