* [PATCH 0/1] x86/boot: clear some fields explicitly
@ 2019-07-24 23:15 john.hubbard
2019-07-24 23:15 ` [PATCH 1/1] " john.hubbard
0 siblings, 1 reply; 16+ messages in thread
From: john.hubbard @ 2019-07-24 23:15 UTC (permalink / raw)
To: H . Peter Anvin
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, LKML, John Hubbard
From: John Hubbard <jhubbard@nvidia.com>
Hi,
I just ran across this on the latest (well, any -rc1) linux.git, while
working on something else. It generates an ugly multiline warning:
In file included from arch/x86/kernel/head64.c:35:
In function ?sanitize_boot_params?,
inlined from ?copy_bootdata? at arch/x86/kernel/head64.c:391:2:
./arch/x86/include/asm/bootparam_utils.h:40:3: warning:
?memset? offset [197, 448] from the object at ?boot_params? is out
of the bounds of referenced subobject ?ext_ramdisk_image? with type
?unsigned int? at offset 192 [-Warray-bounds]
40 | memset(&boot_params->ext_ramdisk_image, 0,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
41 | (char *)&boot_params->efi_info -
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
42 | (char *)&boot_params->ext_ramdisk_image);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/bootparam_utils.h:43:3: warning: ?memset?
offset [493, 497] from the object at ?boot_params? is out of the
bounds of referenced subobject ?kbd_status? with type
?unsigned char? at offset 491 [-Warray-bounds]
43 | memset(&boot_params->kbd_status, 0,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
44 | (char *)&boot_params->hdr -
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~
45 | (char *)&boot_params->kbd_status);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It looks like this hasn't been touched since 2013, so maybe gcc is
just getting more agressive about warnings. Well, we know it is, actually.
Because struct boot_params is __packed__, normal variable
variable assignment will work just as well as a memset here.
Change three u32 fields to be cleared to zero that way, and
just memset the _pad4 field.
This clears up the build warnings for me.
Testing: just did a basic boot test on my x86_64 machine, no problems
seen. Not that that tests much, but it's something.
John Hubbard (1):
x86/boot: clear some fields explicitly
arch/x86/include/asm/bootparam_utils.h | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-24 23:15 [PATCH 0/1] x86/boot: clear some fields explicitly john.hubbard @ 2019-07-24 23:15 ` john.hubbard 2019-07-25 2:12 ` hpa 0 siblings, 1 reply; 16+ messages in thread From: john.hubbard @ 2019-07-24 23:15 UTC (permalink / raw) To: H . Peter Anvin Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, LKML, John Hubbard From: John Hubbard <jhubbard@nvidia.com> Recent gcc compilers (gcc 9.1) generate warnings about an out of bounds memset, if you trying memset across several fields of a struct. This generated a couple of warnings on x86_64 builds. Because struct boot_params is __packed__, normal variable variable assignment will work just as well as a memset here. Change three u32 fields to be cleared to zero that way, and just memset the _pad4 field. This clears up the build warnings for me. Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- arch/x86/include/asm/bootparam_utils.h | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h index 101eb944f13c..4df87d4a043b 100644 --- a/arch/x86/include/asm/bootparam_utils.h +++ b/arch/x86/include/asm/bootparam_utils.h @@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params *boot_params) if (boot_params->sentinel) { /* fields in boot_params are left uninitialized, clear them */ boot_params->acpi_rsdp_addr = 0; - memset(&boot_params->ext_ramdisk_image, 0, - (char *)&boot_params->efi_info - - (char *)&boot_params->ext_ramdisk_image); - memset(&boot_params->kbd_status, 0, - (char *)&boot_params->hdr - - (char *)&boot_params->kbd_status); + boot_params->ext_ramdisk_image = 0; + boot_params->ext_ramdisk_size = 0; + boot_params->ext_cmd_line_ptr = 0; + + memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4)); memset(&boot_params->_pad7[0], 0, (char *)&boot_params->edd_mbr_sig_buffer[0] - (char *)&boot_params->_pad7[0]); -- 2.22.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-24 23:15 ` [PATCH 1/1] " john.hubbard @ 2019-07-25 2:12 ` hpa 2019-07-25 6:49 ` John Hubbard 2019-07-25 7:22 ` Thomas Gleixner 0 siblings, 2 replies; 16+ messages in thread From: hpa @ 2019-07-25 2:12 UTC (permalink / raw) To: john.hubbard Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, LKML, John Hubbard On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote: >From: John Hubbard <jhubbard@nvidia.com> > >Recent gcc compilers (gcc 9.1) generate warnings about an >out of bounds memset, if you trying memset across several fields >of a struct. This generated a couple of warnings on x86_64 builds. > >Because struct boot_params is __packed__, normal variable >variable assignment will work just as well as a memset here. >Change three u32 fields to be cleared to zero that way, and >just memset the _pad4 field. > >This clears up the build warnings for me. > >Signed-off-by: John Hubbard <jhubbard@nvidia.com> >--- > arch/x86/include/asm/bootparam_utils.h | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > >diff --git a/arch/x86/include/asm/bootparam_utils.h >b/arch/x86/include/asm/bootparam_utils.h >index 101eb944f13c..4df87d4a043b 100644 >--- a/arch/x86/include/asm/bootparam_utils.h >+++ b/arch/x86/include/asm/bootparam_utils.h >@@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params >*boot_params) > if (boot_params->sentinel) { > /* fields in boot_params are left uninitialized, clear them */ > boot_params->acpi_rsdp_addr = 0; >- memset(&boot_params->ext_ramdisk_image, 0, >- (char *)&boot_params->efi_info - >- (char *)&boot_params->ext_ramdisk_image); >- memset(&boot_params->kbd_status, 0, >- (char *)&boot_params->hdr - >- (char *)&boot_params->kbd_status); >+ boot_params->ext_ramdisk_image = 0; >+ boot_params->ext_ramdisk_size = 0; >+ boot_params->ext_cmd_line_ptr = 0; >+ >+ memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4)); > memset(&boot_params->_pad7[0], 0, > (char *)&boot_params->edd_mbr_sig_buffer[0] - > (char *)&boot_params->_pad7[0]); The problem with this is that it will break silently when changes are made to this structure. So, that is a NAK from me. -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 2:12 ` hpa @ 2019-07-25 6:49 ` John Hubbard 2019-07-25 7:22 ` Thomas Gleixner 1 sibling, 0 replies; 16+ messages in thread From: John Hubbard @ 2019-07-25 6:49 UTC (permalink / raw) To: hpa, john.hubbard Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86, LKML On 7/24/19 7:12 PM, hpa@zytor.com wrote: > On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote: >> From: John Hubbard <jhubbard@nvidia.com> ... >> + boot_params->ext_ramdisk_image = 0; >> + boot_params->ext_ramdisk_size = 0; >> + boot_params->ext_cmd_line_ptr = 0; >> + >> + memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4)); >> memset(&boot_params->_pad7[0], 0, >> (char *)&boot_params->edd_mbr_sig_buffer[0] - >> (char *)&boot_params->_pad7[0]); > > The problem with this is that it will break silently when changes are made to this structure. > > So, that is a NAK from me. > Understood. It occurs to me, though, that it would be trivial to just add build time assertions to check a few struct member offset values, and fail out if they changed. That would give us everything: warnings-free builds, and no silent failures. Thoughts? thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 2:12 ` hpa 2019-07-25 6:49 ` John Hubbard @ 2019-07-25 7:22 ` Thomas Gleixner 2019-07-25 20:33 ` John Hubbard 2019-07-25 20:38 ` H. Peter Anvin 1 sibling, 2 replies; 16+ messages in thread From: Thomas Gleixner @ 2019-07-25 7:22 UTC (permalink / raw) To: hpa; +Cc: john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML, John Hubbard On Wed, 24 Jul 2019, hpa@zytor.com wrote: > On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote: > >From: John Hubbard <jhubbard@nvidia.com> > > > >Recent gcc compilers (gcc 9.1) generate warnings about an > >out of bounds memset, if you trying memset across several fields > >of a struct. This generated a couple of warnings on x86_64 builds. > > > >Because struct boot_params is __packed__, normal variable > >variable assignment will work just as well as a memset here. > >Change three u32 fields to be cleared to zero that way, and > >just memset the _pad4 field. > > > >This clears up the build warnings for me. > > > >Signed-off-by: John Hubbard <jhubbard@nvidia.com> > >--- > > arch/x86/include/asm/bootparam_utils.h | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > >diff --git a/arch/x86/include/asm/bootparam_utils.h > >b/arch/x86/include/asm/bootparam_utils.h > >index 101eb944f13c..4df87d4a043b 100644 > >--- a/arch/x86/include/asm/bootparam_utils.h > >+++ b/arch/x86/include/asm/bootparam_utils.h > >@@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params > >*boot_params) > > if (boot_params->sentinel) { > > /* fields in boot_params are left uninitialized, clear them */ > > boot_params->acpi_rsdp_addr = 0; > >- memset(&boot_params->ext_ramdisk_image, 0, > >- (char *)&boot_params->efi_info - > >- (char *)&boot_params->ext_ramdisk_image); > >- memset(&boot_params->kbd_status, 0, > >- (char *)&boot_params->hdr - > >- (char *)&boot_params->kbd_status); > >+ boot_params->ext_ramdisk_image = 0; > >+ boot_params->ext_ramdisk_size = 0; > >+ boot_params->ext_cmd_line_ptr = 0; > >+ > >+ memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4)); > > memset(&boot_params->_pad7[0], 0, > > (char *)&boot_params->edd_mbr_sig_buffer[0] - > > (char *)&boot_params->_pad7[0]); > > The problem with this is that it will break silently when changes are > made to this structure. That's not really the worst problem. Changes to that struct which touch any of the to be cleared ranges will break anyway if not handled correctly in the sanitizer function. What's worse is that the patch is broken. It 'clears' the build warnings, but not all the fields which have been cleared before: It removes the clearing of the range between kbd_status and hdr without any replacement. It neither clears edid_info. The above approach is doomed and if we have to handle this GCC0.1 madness then this needs to be done smarter. Something like the completely untested thing below. Thanks, tglx 8<-------------- diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h index 101eb944f13c..f5bc4c01b66b 100644 --- a/arch/x86/include/asm/bootparam_utils.h +++ b/arch/x86/include/asm/bootparam_utils.h @@ -9,6 +9,18 @@ * add completing #includes to make it standalone. */ +struct boot_params_clear { + unsigned int offs; + unsigned int len; +}; + +#define BOOT_PARAM_CLEAR(start, end) \ +{ \ + .offs = offsetof(struct boot_params, start), \ + .len = offsetof(struct boot_params, end) - \ + offsetof(struct boot_params, start), \ +} + /* * Deal with bootloaders which fail to initialize unknown fields in * boot_params to zero. The list fields in this list are taken from @@ -20,7 +32,19 @@ */ static void sanitize_boot_params(struct boot_params *boot_params) { - /* + const struct boot_params_clear toclear[] = { + BOOT_PARAM_CLEAR(acpi_rdsp_addr, _pad3), + BOOT_PARAM_CLEAR(ext_ramdisk_image, efi_info), + BOOT_PARAM_CLEAR(kbd_status, hdr), + BOOT_PARAM_CLEAR(_pad7, edd_mbr_sig_buffer), + BOOT_PARAM_CLEAR(_pad8, eddbuf), + { + .offs = offsetof(struct boot_params, _pad9), + .len = sizeof(boot_params->_pad9), + }, + } + + /* * IMPORTANT NOTE TO BOOTLOADER AUTHORS: do not simply clear * this field. The purpose of this field is to guarantee * compliance with the x86 boot spec located in @@ -36,20 +60,11 @@ static void sanitize_boot_params(struct boot_params *boot_params) */ if (boot_params->sentinel) { /* fields in boot_params are left uninitialized, clear them */ - boot_params->acpi_rsdp_addr = 0; - memset(&boot_params->ext_ramdisk_image, 0, - (char *)&boot_params->efi_info - - (char *)&boot_params->ext_ramdisk_image); - memset(&boot_params->kbd_status, 0, - (char *)&boot_params->hdr - - (char *)&boot_params->kbd_status); - memset(&boot_params->_pad7[0], 0, - (char *)&boot_params->edd_mbr_sig_buffer[0] - - (char *)&boot_params->_pad7[0]); - memset(&boot_params->_pad8[0], 0, - (char *)&boot_params->eddbuf[0] - - (char *)&boot_params->_pad8[0]); - memset(&boot_params->_pad9[0], 0, sizeof(boot_params->_pad9)); + char *p = (char *) boot_params; + int i; + + for (i = 0; i < ARRAY_SIZE(toclear); i++) + memset(p + toclear[i].start, 0, toclear[i].len); } } ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 7:22 ` Thomas Gleixner @ 2019-07-25 20:33 ` John Hubbard 2019-07-25 21:48 ` Thomas Gleixner 2019-07-25 20:38 ` H. Peter Anvin 1 sibling, 1 reply; 16+ messages in thread From: John Hubbard @ 2019-07-25 20:33 UTC (permalink / raw) To: Thomas Gleixner, hpa Cc: john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On 7/25/19 12:22 AM, Thomas Gleixner wrote: > On Wed, 24 Jul 2019, hpa@zytor.com wrote: >> On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote: >>> From: John Hubbard <jhubbard@nvidia.com> >>> >>> Recent gcc compilers (gcc 9.1) generate warnings about an >>> out of bounds memset, if you trying memset across several fields >>> of a struct. This generated a couple of warnings on x86_64 builds. >>> >>> Because struct boot_params is __packed__, normal variable >>> variable assignment will work just as well as a memset here. >>> Change three u32 fields to be cleared to zero that way, and >>> just memset the _pad4 field. >>> >>> This clears up the build warnings for me. >>> >>> Signed-off-by: John Hubbard <jhubbard@nvidia.com> >>> --- >>> arch/x86/include/asm/bootparam_utils.h | 11 +++++------ >>> 1 file changed, 5 insertions(+), 6 deletions(-) >>> >>> diff --git a/arch/x86/include/asm/bootparam_utils.h >>> b/arch/x86/include/asm/bootparam_utils.h >>> index 101eb944f13c..4df87d4a043b 100644 >>> --- a/arch/x86/include/asm/bootparam_utils.h >>> +++ b/arch/x86/include/asm/bootparam_utils.h >>> @@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params >>> *boot_params) >>> if (boot_params->sentinel) { >>> /* fields in boot_params are left uninitialized, clear them */ >>> boot_params->acpi_rsdp_addr = 0; >>> - memset(&boot_params->ext_ramdisk_image, 0, >>> - (char *)&boot_params->efi_info - >>> - (char *)&boot_params->ext_ramdisk_image); >>> - memset(&boot_params->kbd_status, 0, >>> - (char *)&boot_params->hdr - >>> - (char *)&boot_params->kbd_status); >>> + boot_params->ext_ramdisk_image = 0; >>> + boot_params->ext_ramdisk_size = 0; >>> + boot_params->ext_cmd_line_ptr = 0; >>> + >>> + memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4)); >>> memset(&boot_params->_pad7[0], 0, >>> (char *)&boot_params->edd_mbr_sig_buffer[0] - >>> (char *)&boot_params->_pad7[0]); >> >> The problem with this is that it will break silently when changes are >> made to this structure. > > That's not really the worst problem. Changes to that struct which touch any > of the to be cleared ranges will break anyway if not handled correctly in > the sanitizer function. > > What's worse is that the patch is broken. It 'clears' the build warnings, > but not all the fields which have been cleared before: > > It removes the clearing of the range between kbd_status and hdr without any > replacement. It neither clears edid_info. Yes. Somehow I left that chunk out. Not my finest hour. > > The above approach is doomed and if we have to handle this GCC0.1 madness > then this needs to be done smarter. Something like the completely untested > thing below. > > Thanks, > > tglx > > 8<-------------- > diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h > index 101eb944f13c..f5bc4c01b66b 100644 > --- a/arch/x86/include/asm/bootparam_utils.h > +++ b/arch/x86/include/asm/bootparam_utils.h > @@ -9,6 +9,18 @@ > * add completing #includes to make it standalone. > */ > > +struct boot_params_clear { > + unsigned int offs; > + unsigned int len; > +}; > + > +#define BOOT_PARAM_CLEAR(start, end) \ > +{ \ > + .offs = offsetof(struct boot_params, start), \ > + .len = offsetof(struct boot_params, end) - \ > + offsetof(struct boot_params, start), \ > +} > + > /* > * Deal with bootloaders which fail to initialize unknown fields in > * boot_params to zero. The list fields in this list are taken from > @@ -20,7 +32,19 @@ > */ > static void sanitize_boot_params(struct boot_params *boot_params) > { > - /* > + const struct boot_params_clear toclear[] = { > + BOOT_PARAM_CLEAR(acpi_rdsp_addr, _pad3), > + BOOT_PARAM_CLEAR(ext_ramdisk_image, efi_info), > + BOOT_PARAM_CLEAR(kbd_status, hdr), > + BOOT_PARAM_CLEAR(_pad7, edd_mbr_sig_buffer), > + BOOT_PARAM_CLEAR(_pad8, eddbuf), > + { > + .offs = offsetof(struct boot_params, _pad9), > + .len = sizeof(boot_params->_pad9), > + }, > + } > + > + /* > * IMPORTANT NOTE TO BOOTLOADER AUTHORS: do not simply clear > * this field. The purpose of this field is to guarantee > * compliance with the x86 boot spec located in > @@ -36,20 +60,11 @@ static void sanitize_boot_params(struct boot_params *boot_params) > */ > if (boot_params->sentinel) { > /* fields in boot_params are left uninitialized, clear them */ > - boot_params->acpi_rsdp_addr = 0; > - memset(&boot_params->ext_ramdisk_image, 0, > - (char *)&boot_params->efi_info - > - (char *)&boot_params->ext_ramdisk_image); > - memset(&boot_params->kbd_status, 0, > - (char *)&boot_params->hdr - > - (char *)&boot_params->kbd_status); > - memset(&boot_params->_pad7[0], 0, > - (char *)&boot_params->edd_mbr_sig_buffer[0] - > - (char *)&boot_params->_pad7[0]); > - memset(&boot_params->_pad8[0], 0, > - (char *)&boot_params->eddbuf[0] - > - (char *)&boot_params->_pad8[0]); > - memset(&boot_params->_pad9[0], 0, sizeof(boot_params->_pad9)); > + char *p = (char *) boot_params; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(toclear); i++) > + memset(p + toclear[i].start, 0, toclear[i].len); > } > } Looks nice. thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 20:33 ` John Hubbard @ 2019-07-25 21:48 ` Thomas Gleixner 2019-07-25 21:57 ` hpa 0 siblings, 1 reply; 16+ messages in thread From: Thomas Gleixner @ 2019-07-25 21:48 UTC (permalink / raw) To: John Hubbard; +Cc: hpa, john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On Thu, 25 Jul 2019, John Hubbard wrote: > On 7/25/19 12:22 AM, Thomas Gleixner wrote: > > It removes the clearing of the range between kbd_status and hdr without any > > replacement. It neither clears edid_info. > > > Yes. Somehow I left that chunk out. Not my finest hour. S*** happens > > + char *p = (char *) boot_params; > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(toclear); i++) > > + memset(p + toclear[i].start, 0, toclear[i].len); > > } > > } > > Looks nice. I have no idea whether it works and I have no cycles either, so I would appreciate it if you could polish it up so we can handle that new fangled GCC "feature" nicely. Alternatively file a bug report to the GCC folks :) But seriously I think it's not completely insane what they are doing and the table based approach is definitely more readable and maintainable than the existing stuff. Thanks, tglx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 21:48 ` Thomas Gleixner @ 2019-07-25 21:57 ` hpa 2019-07-25 22:03 ` Thomas Gleixner 0 siblings, 1 reply; 16+ messages in thread From: hpa @ 2019-07-25 21:57 UTC (permalink / raw) To: Thomas Gleixner, John Hubbard Cc: john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote: >On Thu, 25 Jul 2019, John Hubbard wrote: >> On 7/25/19 12:22 AM, Thomas Gleixner wrote: >> > It removes the clearing of the range between kbd_status and hdr >without any >> > replacement. It neither clears edid_info. >> >> >> Yes. Somehow I left that chunk out. Not my finest hour. > >S*** happens > >> > + char *p = (char *) boot_params; >> > + int i; >> > + >> > + for (i = 0; i < ARRAY_SIZE(toclear); i++) >> > + memset(p + toclear[i].start, 0, toclear[i].len); >> > } >> > } >> >> Looks nice. > >I have no idea whether it works and I have no cycles either, so I would >appreciate it if you could polish it up so we can handle that new >fangled >GCC "feature" nicely. > >Alternatively file a bug report to the GCC folks :) > >But seriously I think it's not completely insane what they are doing >and >the table based approach is definitely more readable and maintainable >than >the existing stuff. > >Thanks, > > tglx Doing this table based does seem like a good idea. -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 21:57 ` hpa @ 2019-07-25 22:03 ` Thomas Gleixner 2019-07-25 22:28 ` H. Peter Anvin 0 siblings, 1 reply; 16+ messages in thread From: Thomas Gleixner @ 2019-07-25 22:03 UTC (permalink / raw) To: hpa; +Cc: John Hubbard, john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On Thu, 25 Jul 2019, hpa@zytor.com wrote: > On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote: > > > > But seriously I think it's not completely insane what they are doing > > and the table based approach is definitely more readable and maintainable > > than the existing stuff. > > Doing this table based does seem like a good idea. The question is whether we use a 'toclear' table or a 'preserve' table. I'd argue that the 'preserve' approach is saner. > Sent from my Android device with K-9 Mail. Please excuse my brevity. I surely excuse the brevity, but the formatting mess which that brevity app creates is not excusable. Thanks, tglx --- Sent from a sane mail client. Nothing to excuse here. All failures are intentional. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 22:03 ` Thomas Gleixner @ 2019-07-25 22:28 ` H. Peter Anvin 2019-07-25 22:37 ` Thomas Gleixner 2019-07-25 22:42 ` John Hubbard 0 siblings, 2 replies; 16+ messages in thread From: H. Peter Anvin @ 2019-07-25 22:28 UTC (permalink / raw) To: Thomas Gleixner Cc: John Hubbard, john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On 7/25/19 3:03 PM, Thomas Gleixner wrote: > On Thu, 25 Jul 2019, hpa@zytor.com wrote: >> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote: >>> >>> But seriously I think it's not completely insane what they are doing >>> and the table based approach is definitely more readable and maintainable >>> than the existing stuff. >> >> Doing this table based does seem like a good idea. > > The question is whether we use a 'toclear' table or a 'preserve' table. I'd > argue that the 'preserve' approach is saner. > I agree. >> Sent from my Android device with K-9 Mail. Please excuse my brevity. > > I surely excuse the brevity, but the formatting mess which that brevity app > creates is not excusable. > I'll try to improve it... -hpa ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 22:28 ` H. Peter Anvin @ 2019-07-25 22:37 ` Thomas Gleixner 2019-07-26 0:36 ` John Hubbard 2019-07-25 22:42 ` John Hubbard 1 sibling, 1 reply; 16+ messages in thread From: Thomas Gleixner @ 2019-07-25 22:37 UTC (permalink / raw) To: H. Peter Anvin Cc: John Hubbard, john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On Thu, 25 Jul 2019, H. Peter Anvin wrote: > On 7/25/19 3:03 PM, Thomas Gleixner wrote: > > On Thu, 25 Jul 2019, hpa@zytor.com wrote: > >> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote: > >>> > >>> But seriously I think it's not completely insane what they are doing > >>> and the table based approach is definitely more readable and maintainable > >>> than the existing stuff. > >> > >> Doing this table based does seem like a good idea. > > > > The question is whether we use a 'toclear' table or a 'preserve' table. I'd > > argue that the 'preserve' approach is saner. > > > I agree. Now we just need to volunteer someone to do that :) > >> Sent from my Android device with K-9 Mail. Please excuse my brevity. > > > > I surely excuse the brevity, but the formatting mess which that brevity app > > creates is not excusable. > > > > I'll try to improve it... SCNR ... ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 22:37 ` Thomas Gleixner @ 2019-07-26 0:36 ` John Hubbard 0 siblings, 0 replies; 16+ messages in thread From: John Hubbard @ 2019-07-26 0:36 UTC (permalink / raw) To: Thomas Gleixner, H. Peter Anvin; +Cc: Ingo Molnar, Borislav Petkov, x86, LKML On 7/25/19 3:37 PM, Thomas Gleixner wrote: > On Thu, 25 Jul 2019, H. Peter Anvin wrote: >> On 7/25/19 3:03 PM, Thomas Gleixner wrote: >>> On Thu, 25 Jul 2019, hpa@zytor.com wrote: >>>> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote: >>>>> >>>>> But seriously I think it's not completely insane what they are doing >>>>> and the table based approach is definitely more readable and maintainable >>>>> than the existing stuff. >>>> >>>> Doing this table based does seem like a good idea. >>> >>> The question is whether we use a 'toclear' table or a 'preserve' table. I'd >>> argue that the 'preserve' approach is saner. >>> >> I agree. > > Now we just need to volunteer someone to do that :) > Happy to jump in and do that, since I have an easy repro of the warning here. In case you missed an earlier response [1], I did have a lingering question about what you had in mind for the "to preserve" approach: [1] https://lore.kernel.org/r/ffd7a9b6-8017-2d2c-c4f7-65563094ccd0@nvidia.com thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 22:28 ` H. Peter Anvin 2019-07-25 22:37 ` Thomas Gleixner @ 2019-07-25 22:42 ` John Hubbard 2019-07-26 7:43 ` Thomas Gleixner 1 sibling, 1 reply; 16+ messages in thread From: John Hubbard @ 2019-07-25 22:42 UTC (permalink / raw) To: H. Peter Anvin, Thomas Gleixner Cc: john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On 7/25/19 3:28 PM, H. Peter Anvin wrote: > On 7/25/19 3:03 PM, Thomas Gleixner wrote: >> On Thu, 25 Jul 2019, hpa@zytor.com wrote: >>> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote: >>>> >>>> But seriously I think it's not completely insane what they are doing >>>> and the table based approach is definitely more readable and maintainable >>>> than the existing stuff. >>> >>> Doing this table based does seem like a good idea. >> >> The question is whether we use a 'toclear' table or a 'preserve' table. I'd >> argue that the 'preserve' approach is saner. >> > > I agree. > OK, I can polish up something and post it, if you can help me with one more quick question: how did you want "to preserve" to work? a) copy out fields to preserve, memset the area to zero, copy back preserved fields? This seems like it would have the same gcc warnings as we have now, due to the requirement to memset a range of a struct... b) Iterate through all fields, memsetting to zero items that are *not* marked "to preserve"? c) Something else? Sorry for the naivete here. I really did read Documentation/x86/boot.rst, honest. :) thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 22:42 ` John Hubbard @ 2019-07-26 7:43 ` Thomas Gleixner 0 siblings, 0 replies; 16+ messages in thread From: Thomas Gleixner @ 2019-07-26 7:43 UTC (permalink / raw) To: John Hubbard Cc: H. Peter Anvin, john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On Thu, 25 Jul 2019, John Hubbard wrote: > On 7/25/19 3:28 PM, H. Peter Anvin wrote: > > On 7/25/19 3:03 PM, Thomas Gleixner wrote: > >> On Thu, 25 Jul 2019, hpa@zytor.com wrote: > >>> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote: > >>>> > >>>> But seriously I think it's not completely insane what they are doing > >>>> and the table based approach is definitely more readable and maintainable > >>>> than the existing stuff. > >>> > >>> Doing this table based does seem like a good idea. > >> > >> The question is whether we use a 'toclear' table or a 'preserve' table. I'd > >> argue that the 'preserve' approach is saner. > >> > > > > I agree. > > > > OK, I can polish up something and post it, if you can help me with one more > quick question: how did you want "to preserve" to work? > > a) copy out fields to preserve, memset the area to zero, copy back preserved > fields? This seems like it would have the same gcc warnings as we have now, > due to the requirement to memset a range of a struct... Use the same trick I used for the toclear variant. #define PRESERVE(m) \ { \ .start = offsetof(m), \ .len = sizeof(m), \ } sanitize_boot_params(bp, scratch) { char *p1 = bp, *p2 = scratch; preserve[] = { PRESERVE(member1), ... PRESERVE(memberN), }; for_each_preserve(pr) memcpy(p2 + pr->start, p1 + pr->start, pr->len) memcpy(bp, scratch, ...); } Thanks, tglx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 7:22 ` Thomas Gleixner 2019-07-25 20:33 ` John Hubbard @ 2019-07-25 20:38 ` H. Peter Anvin 2019-07-25 20:44 ` John Hubbard 1 sibling, 1 reply; 16+ messages in thread From: H. Peter Anvin @ 2019-07-25 20:38 UTC (permalink / raw) To: Thomas Gleixner Cc: john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML, John Hubbard On 7/25/19 12:22 AM, Thomas Gleixner wrote: >> >> The problem with this is that it will break silently when changes are >> made to this structure. > > That's not really the worst problem. Changes to that struct which touch any > of the to be cleared ranges will break anyway if not handled correctly in > the sanitizer function. > Not really... that's kind of the point (the cleared ranges are cleared explicitly because the boot loader failed to do so, so zeroing them is what the boot loader should have done.) The most correct way to address this would be to have an explicit list of members to be *preserved* even if the sentinel triggers. The easy way would be to put in a suitable cast to clear the warning -- I would not be surprised if an explicit cast to something like (void *) would quiet the warning, or else (yuck) put in an explicit (well-commented) #pragma to shut it up. -hpa ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] x86/boot: clear some fields explicitly 2019-07-25 20:38 ` H. Peter Anvin @ 2019-07-25 20:44 ` John Hubbard 0 siblings, 0 replies; 16+ messages in thread From: John Hubbard @ 2019-07-25 20:44 UTC (permalink / raw) To: H. Peter Anvin, Thomas Gleixner Cc: john.hubbard, Ingo Molnar, Borislav Petkov, x86, LKML On 7/25/19 1:38 PM, H. Peter Anvin wrote: > On 7/25/19 12:22 AM, Thomas Gleixner wrote: > The easy way would be to put in a suitable cast to clear the warning -- I > would not be surprised if an explicit cast to something like (void *) would > quiet the warning, or else (yuck) put in an explicit (well-commented) #pragma > to shut it up. > I wish. The first thing I tried was a (void*) cast, and the second thing was declaring a void pointer instead. But the new compiler is fiendishly clever, and figured me out in both cases. The #pragma I haven't tried, it seems like a bit too far. thanks, -- John Hubbard NVIDIA ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-07-26 7:43 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-24 23:15 [PATCH 0/1] x86/boot: clear some fields explicitly john.hubbard 2019-07-24 23:15 ` [PATCH 1/1] " john.hubbard 2019-07-25 2:12 ` hpa 2019-07-25 6:49 ` John Hubbard 2019-07-25 7:22 ` Thomas Gleixner 2019-07-25 20:33 ` John Hubbard 2019-07-25 21:48 ` Thomas Gleixner 2019-07-25 21:57 ` hpa 2019-07-25 22:03 ` Thomas Gleixner 2019-07-25 22:28 ` H. Peter Anvin 2019-07-25 22:37 ` Thomas Gleixner 2019-07-26 0:36 ` John Hubbard 2019-07-25 22:42 ` John Hubbard 2019-07-26 7:43 ` Thomas Gleixner 2019-07-25 20:38 ` H. Peter Anvin 2019-07-25 20:44 ` John Hubbard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.