From: John Hubbard <[email protected]>
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.
Fix this by explicitly saving the fields in struct boot_params
that are intended to be preserved, and zeroing all the rest.
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
arch/x86/include/asm/bootparam_utils.h | 62 +++++++++++++++++++-------
1 file changed, 47 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 101eb944f13c..514aee24b8de 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -18,6 +18,20 @@
* Note: efi_info is commonly left uninitialized, but that field has a
* private magic, so it is better to leave it unchanged.
*/
+
+#define sizeof_mbr(type, member) ({ sizeof(((type *)0)->member); })
+
+#define BOOT_PARAM_PRESERVE(struct_member) \
+ { \
+ .start = offsetof(struct boot_params, struct_member), \
+ .len = sizeof_mbr(struct boot_params, struct_member), \
+ }
+
+struct boot_params_to_save {
+ unsigned int start;
+ unsigned int len;
+};
+
static void sanitize_boot_params(struct boot_params *boot_params)
{
/*
@@ -35,21 +49,39 @@ static void sanitize_boot_params(struct boot_params *boot_params)
* problems again.
*/
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));
+ static struct boot_params scratch;
+ char *bp_base = (char *)boot_params;
+ char *save_base = (char *)&scratch;
+ int i;
+
+ const struct boot_params_to_save to_save[] = {
+ BOOT_PARAM_PRESERVE(screen_info),
+ BOOT_PARAM_PRESERVE(apm_bios_info),
+ BOOT_PARAM_PRESERVE(tboot_addr),
+ BOOT_PARAM_PRESERVE(ist_info),
+ BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
+ BOOT_PARAM_PRESERVE(hd0_info),
+ BOOT_PARAM_PRESERVE(hd1_info),
+ BOOT_PARAM_PRESERVE(sys_desc_table),
+ BOOT_PARAM_PRESERVE(olpc_ofw_header),
+ BOOT_PARAM_PRESERVE(efi_info),
+ BOOT_PARAM_PRESERVE(alt_mem_k),
+ BOOT_PARAM_PRESERVE(scratch),
+ BOOT_PARAM_PRESERVE(e820_entries),
+ BOOT_PARAM_PRESERVE(eddbuf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(e820_table),
+ BOOT_PARAM_PRESERVE(eddbuf),
+ };
+
+ memset(&scratch, 0, sizeof(scratch));
+
+ for (i = 0; i < ARRAY_SIZE(to_save); i++)
+ memcpy(save_base + to_save[i].start,
+ bp_base + to_save[i].start, to_save[i].len);
+
+ memcpy(boot_params, save_base, sizeof(*boot_params));
}
}
--
2.22.0
From: [email protected]
> Sent: 31 July 2019 06:46
>
> 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.
>
> Fix this by explicitly saving the fields in struct boot_params
> that are intended to be preserved, and zeroing all the rest.
>
> Suggested-by: Thomas Gleixner <[email protected]>
> Suggested-by: H. Peter Anvin <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> arch/x86/include/asm/bootparam_utils.h | 62 +++++++++++++++++++-------
> 1 file changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> index 101eb944f13c..514aee24b8de 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -18,6 +18,20 @@
> * Note: efi_info is commonly left uninitialized, but that field has a
> * private magic, so it is better to leave it unchanged.
> */
> +
> +#define sizeof_mbr(type, member) ({ sizeof(((type *)0)->member); })
> +
> +#define BOOT_PARAM_PRESERVE(struct_member) \
> + { \
> + .start = offsetof(struct boot_params, struct_member), \
> + .len = sizeof_mbr(struct boot_params, struct_member), \
> + }
> +
> +struct boot_params_to_save {
> + unsigned int start;
> + unsigned int len;
> +};
> +
> static void sanitize_boot_params(struct boot_params *boot_params)
> {
> /*
> @@ -35,21 +49,39 @@ static void sanitize_boot_params(struct boot_params *boot_params)
> * problems again.
> */
> 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));
...
How about replacing the above first using:
#define zero_struct_fields(ptr, from, to) memset(&ptr->from, 0, (char *)&ptr->to - (char *)&ptr->from)
zero_struct_fields(boot_params, ext_ramdisk_image, efi_info);
...
Which is absolutely identical to the original code.
The replacing the define with:
#define so(s, m) offsetof(typeof(*s), m)
#define zero_struct_fields(ptr, from, to) memset((char *)ptr + so(ptr, from), 0, so(ptr, to) - so(ptr, from))
which gcc probably doesn't complain about, but should generate identical code again.
There might be an existing define for so().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Commit-ID: 610666f0581557944c3abec93a7c125b8303442c
Gitweb: https://git.kernel.org/tip/610666f0581557944c3abec93a7c125b8303442c
Author: John Hubbard <[email protected]>
AuthorDate: Tue, 30 Jul 2019 22:46:27 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 7 Aug 2019 15:16:04 +0200
x86/boot: Save fields explicitly, zero out everything else
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.
Fix this by explicitly saving the fields in struct boot_params
that are intended to be preserved, and zeroing all the rest.
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/bootparam_utils.h | 63 ++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 101eb944f13c..f5e90a849bca 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -18,6 +18,20 @@
* Note: efi_info is commonly left uninitialized, but that field has a
* private magic, so it is better to leave it unchanged.
*/
+
+#define sizeof_mbr(type, member) ({ sizeof(((type *)0)->member); })
+
+#define BOOT_PARAM_PRESERVE(struct_member) \
+ { \
+ .start = offsetof(struct boot_params, struct_member), \
+ .len = sizeof_mbr(struct boot_params, struct_member), \
+ }
+
+struct boot_params_to_save {
+ unsigned int start;
+ unsigned int len;
+};
+
static void sanitize_boot_params(struct boot_params *boot_params)
{
/*
@@ -35,21 +49,40 @@ static void sanitize_boot_params(struct boot_params *boot_params)
* problems again.
*/
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));
+ static struct boot_params scratch;
+ char *bp_base = (char *)boot_params;
+ char *save_base = (char *)&scratch;
+ int i;
+
+ const struct boot_params_to_save to_save[] = {
+ BOOT_PARAM_PRESERVE(screen_info),
+ BOOT_PARAM_PRESERVE(apm_bios_info),
+ BOOT_PARAM_PRESERVE(tboot_addr),
+ BOOT_PARAM_PRESERVE(ist_info),
+ BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
+ BOOT_PARAM_PRESERVE(hd0_info),
+ BOOT_PARAM_PRESERVE(hd1_info),
+ BOOT_PARAM_PRESERVE(sys_desc_table),
+ BOOT_PARAM_PRESERVE(olpc_ofw_header),
+ BOOT_PARAM_PRESERVE(efi_info),
+ BOOT_PARAM_PRESERVE(alt_mem_k),
+ BOOT_PARAM_PRESERVE(scratch),
+ BOOT_PARAM_PRESERVE(e820_entries),
+ BOOT_PARAM_PRESERVE(eddbuf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(e820_table),
+ BOOT_PARAM_PRESERVE(eddbuf),
+ };
+
+ memset(&scratch, 0, sizeof(scratch));
+
+ for (i = 0; i < ARRAY_SIZE(to_save); i++) {
+ memcpy(save_base + to_save[i].start,
+ bp_base + to_save[i].start, to_save[i].len);
+ }
+
+ memcpy(boot_params, save_base, sizeof(*boot_params));
}
}
Commit-ID: a156cadef2fe445ac423670eace517b39a01ccd0
Gitweb: https://git.kernel.org/tip/a156cadef2fe445ac423670eace517b39a01ccd0
Author: John Hubbard <[email protected]>
AuthorDate: Tue, 30 Jul 2019 22:46:27 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 7 Aug 2019 15:22:53 +0200
x86/boot: Save fields explicitly, zero out everything else
Recent gcc compilers (gcc 9.1) generate warnings about an out of bounds
memset, if the memset goes accross several fields of a struct. This
generated a couple of warnings on x86_64 builds in sanitize_boot_params().
Fix this by explicitly saving the fields in struct boot_params
that are intended to be preserved, and zeroing all the rest.
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/bootparam_utils.h | 63 ++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 101eb944f13c..f5e90a849bca 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -18,6 +18,20 @@
* Note: efi_info is commonly left uninitialized, but that field has a
* private magic, so it is better to leave it unchanged.
*/
+
+#define sizeof_mbr(type, member) ({ sizeof(((type *)0)->member); })
+
+#define BOOT_PARAM_PRESERVE(struct_member) \
+ { \
+ .start = offsetof(struct boot_params, struct_member), \
+ .len = sizeof_mbr(struct boot_params, struct_member), \
+ }
+
+struct boot_params_to_save {
+ unsigned int start;
+ unsigned int len;
+};
+
static void sanitize_boot_params(struct boot_params *boot_params)
{
/*
@@ -35,21 +49,40 @@ static void sanitize_boot_params(struct boot_params *boot_params)
* problems again.
*/
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));
+ static struct boot_params scratch;
+ char *bp_base = (char *)boot_params;
+ char *save_base = (char *)&scratch;
+ int i;
+
+ const struct boot_params_to_save to_save[] = {
+ BOOT_PARAM_PRESERVE(screen_info),
+ BOOT_PARAM_PRESERVE(apm_bios_info),
+ BOOT_PARAM_PRESERVE(tboot_addr),
+ BOOT_PARAM_PRESERVE(ist_info),
+ BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
+ BOOT_PARAM_PRESERVE(hd0_info),
+ BOOT_PARAM_PRESERVE(hd1_info),
+ BOOT_PARAM_PRESERVE(sys_desc_table),
+ BOOT_PARAM_PRESERVE(olpc_ofw_header),
+ BOOT_PARAM_PRESERVE(efi_info),
+ BOOT_PARAM_PRESERVE(alt_mem_k),
+ BOOT_PARAM_PRESERVE(scratch),
+ BOOT_PARAM_PRESERVE(e820_entries),
+ BOOT_PARAM_PRESERVE(eddbuf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(e820_table),
+ BOOT_PARAM_PRESERVE(eddbuf),
+ };
+
+ memset(&scratch, 0, sizeof(scratch));
+
+ for (i = 0; i < ARRAY_SIZE(to_save); i++) {
+ memcpy(save_base + to_save[i].start,
+ bp_base + to_save[i].start, to_save[i].len);
+ }
+
+ memcpy(boot_params, save_base, sizeof(*boot_params));
}
}
On 8/7/19 4:41 AM, David Laight wrote:
> From: [email protected]
>> Sent: 31 July 2019 06:46
...
>> 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));
> ...
>
> How about replacing the above first using:
> #define zero_struct_fields(ptr, from, to) memset(&ptr->from, 0, (char *)&ptr->to - (char *)&ptr->from)
> zero_struct_fields(boot_params, ext_ramdisk_image, efi_info);
> ...
> Which is absolutely identical to the original code.
>
> The replacing the define with:
> #define so(s, m) offsetof(typeof(*s), m)
> #define zero_struct_fields(ptr, from, to) memset((char *)ptr + so(ptr, from), 0, so(ptr, to) - so(ptr, from))
> which gcc probably doesn't complain about, but should generate identical code again.
> There might be an existing define for so().
>
Hi David,
There was discussion about that [1], but preference ending up being to
flip this around, in order to more closely match the original intent
of this function (zero out everything except for certain carefully
selected fields), and to therefore be more likely to keep working if
fields are added.
[1] https://lore.kernel.org/lkml/[email protected]/
thanks,
--
John Hubbard
NVIDIA
On 31/07/2019 06:46, [email protected] wrote:
> From: John Hubbard <[email protected]>
>
> 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.
>
> Fix this by explicitly saving the fields in struct boot_params
> that are intended to be preserved, and zeroing all the rest.
>
I applied John's patch below to v5.3-rc3-285-gecb095bff5d4 and have been running the resultant kernel for two days now,
including 7 or 8 cold starts and reboots. The warnings that were produced by gcc9 are no longer emitted and, other than
a pre-existing problem (no network after resume from suspend or hibernate which I will investigate and, if necessary,
report later today), the kernel has supported my typical day to day activities (building software, email, browsing,
listening to music, watching video) without problem.
Tested-by: Chris Clayton <[email protected]>
> Suggested-by: Thomas Gleixner <[email protected]>
> Suggested-by: H. Peter Anvin <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> arch/x86/include/asm/bootparam_utils.h | 62 +++++++++++++++++++-------
> 1 file changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> index 101eb944f13c..514aee24b8de 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -18,6 +18,20 @@
> * Note: efi_info is commonly left uninitialized, but that field has a
> * private magic, so it is better to leave it unchanged.
> */
> +
> +#define sizeof_mbr(type, member) ({ sizeof(((type *)0)->member); })
> +
> +#define BOOT_PARAM_PRESERVE(struct_member) \
> + { \
> + .start = offsetof(struct boot_params, struct_member), \
> + .len = sizeof_mbr(struct boot_params, struct_member), \
> + }
> +
> +struct boot_params_to_save {
> + unsigned int start;
> + unsigned int len;
> +};
> +
> static void sanitize_boot_params(struct boot_params *boot_params)
> {
> /*
> @@ -35,21 +49,39 @@ static void sanitize_boot_params(struct boot_params *boot_params)
> * problems again.
> */
> 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));
> + static struct boot_params scratch;
> + char *bp_base = (char *)boot_params;
> + char *save_base = (char *)&scratch;
> + int i;
> +
> + const struct boot_params_to_save to_save[] = {
> + BOOT_PARAM_PRESERVE(screen_info),
> + BOOT_PARAM_PRESERVE(apm_bios_info),
> + BOOT_PARAM_PRESERVE(tboot_addr),
> + BOOT_PARAM_PRESERVE(ist_info),
> + BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> + BOOT_PARAM_PRESERVE(hd0_info),
> + BOOT_PARAM_PRESERVE(hd1_info),
> + BOOT_PARAM_PRESERVE(sys_desc_table),
> + BOOT_PARAM_PRESERVE(olpc_ofw_header),
> + BOOT_PARAM_PRESERVE(efi_info),
> + BOOT_PARAM_PRESERVE(alt_mem_k),
> + BOOT_PARAM_PRESERVE(scratch),
> + BOOT_PARAM_PRESERVE(e820_entries),
> + BOOT_PARAM_PRESERVE(eddbuf_entries),
> + BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> + BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> + BOOT_PARAM_PRESERVE(e820_table),
> + BOOT_PARAM_PRESERVE(eddbuf),
> + };
> +
> + memset(&scratch, 0, sizeof(scratch));
> +
> + for (i = 0; i < ARRAY_SIZE(to_save); i++)
> + memcpy(save_base + to_save[i].start,
> + bp_base + to_save[i].start, to_save[i].len);
> +
> + memcpy(boot_params, save_base, sizeof(*boot_params));
> }
> }
>
>
Commit-ID: a90118c445cc7f07781de26a9684d4ec58bfcfd1
Gitweb: https://git.kernel.org/tip/a90118c445cc7f07781de26a9684d4ec58bfcfd1
Author: John Hubbard <[email protected]>
AuthorDate: Tue, 30 Jul 2019 22:46:27 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Fri, 16 Aug 2019 14:20:00 +0200
x86/boot: Save fields explicitly, zero out everything else
Recent gcc compilers (gcc 9.1) generate warnings about an out of bounds
memset, if the memset goes accross several fields of a struct. This
generated a couple of warnings on x86_64 builds in sanitize_boot_params().
Fix this by explicitly saving the fields in struct boot_params
that are intended to be preserved, and zeroing all the rest.
[ tglx: Tagged for stable as it breaks the warning free build there as well ]
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/bootparam_utils.h | 63 ++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 101eb944f13c..f5e90a849bca 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -18,6 +18,20 @@
* Note: efi_info is commonly left uninitialized, but that field has a
* private magic, so it is better to leave it unchanged.
*/
+
+#define sizeof_mbr(type, member) ({ sizeof(((type *)0)->member); })
+
+#define BOOT_PARAM_PRESERVE(struct_member) \
+ { \
+ .start = offsetof(struct boot_params, struct_member), \
+ .len = sizeof_mbr(struct boot_params, struct_member), \
+ }
+
+struct boot_params_to_save {
+ unsigned int start;
+ unsigned int len;
+};
+
static void sanitize_boot_params(struct boot_params *boot_params)
{
/*
@@ -35,21 +49,40 @@ static void sanitize_boot_params(struct boot_params *boot_params)
* problems again.
*/
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));
+ static struct boot_params scratch;
+ char *bp_base = (char *)boot_params;
+ char *save_base = (char *)&scratch;
+ int i;
+
+ const struct boot_params_to_save to_save[] = {
+ BOOT_PARAM_PRESERVE(screen_info),
+ BOOT_PARAM_PRESERVE(apm_bios_info),
+ BOOT_PARAM_PRESERVE(tboot_addr),
+ BOOT_PARAM_PRESERVE(ist_info),
+ BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
+ BOOT_PARAM_PRESERVE(hd0_info),
+ BOOT_PARAM_PRESERVE(hd1_info),
+ BOOT_PARAM_PRESERVE(sys_desc_table),
+ BOOT_PARAM_PRESERVE(olpc_ofw_header),
+ BOOT_PARAM_PRESERVE(efi_info),
+ BOOT_PARAM_PRESERVE(alt_mem_k),
+ BOOT_PARAM_PRESERVE(scratch),
+ BOOT_PARAM_PRESERVE(e820_entries),
+ BOOT_PARAM_PRESERVE(eddbuf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
+ BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(e820_table),
+ BOOT_PARAM_PRESERVE(eddbuf),
+ };
+
+ memset(&scratch, 0, sizeof(scratch));
+
+ for (i = 0; i < ARRAY_SIZE(to_save); i++) {
+ memcpy(save_base + to_save[i].start,
+ bp_base + to_save[i].start, to_save[i].len);
+ }
+
+ memcpy(boot_params, save_base, sizeof(*boot_params));
}
}
From: "John S. Gruber" <[email protected]>
commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
else") now zeros the secure boot information passed by the boot loader or
by the kernel's efi handover mechanism.
Include boot-params.secure_boot in the preserve field list.
Signed-off-by: John S. Gruber <[email protected]>
---
I noted a change in my computers between running signed 5.3-rc4 and 5.3-rc6
with signed kernels using the efi handoff protocol with grub. The kernel
log message "Secure boot enabled" becomes "Secure boot could not be
determined". The efi_main function in arch/x86/boot/compressed/eboot.c sets
this field early but it is subsequently zeroed by the above referenced commit
in the file arch/x86/include/asm/bootparam_utils.h
Applies to 5.3-rc6.
arch/x86/include/asm/bootparam_utils.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/bootparam_utils.h
b/arch/x86/include/asm/bootparam_utils.h
index 9e5f3c7..981fe92 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params
*boot_params)
BOOT_PARAM_PRESERVE(eddbuf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(secure_boot),
BOOT_PARAM_PRESERVE(hdr),
BOOT_PARAM_PRESERVE(e820_table),
BOOT_PARAM_PRESERVE(eddbuf),
--
2.7.4
On 9/1/19 8:38 AM, John S Gruber wrote:
> From: "John S. Gruber" <[email protected]>
>
> commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
> else") now zeros the secure boot information passed by the boot loader or
> by the kernel's efi handover mechanism.
>
> Include boot-params.secure_boot in the preserve field list.
>
> Signed-off-by: John S. Gruber <[email protected]>
> ---
>
> I noted a change in my computers between running signed 5.3-rc4 and 5.3-rc6
> with signed kernels using the efi handoff protocol with grub. The kernel
> log message "Secure boot enabled" becomes "Secure boot could not be
> determined". The efi_main function in arch/x86/boot/compressed/eboot.c sets
> this field early but it is subsequently zeroed by the above referenced commit
> in the file arch/x86/include/asm/bootparam_utils.h
>
> Applies to 5.3-rc6.
>
Hi,
The fix itself looks good, so you can add:
Reviewed-by: John Hubbard <[email protected]>
...but note that the commit description should get a few tweaks:
1. Your description above is actually well-suited for the commit log,
so please add that in. Especially the symptoms are desirable to have
on record.
2. This should Cc: [email protected], because the whole thing
made it into -stable and those kernels need this fix.
3. Also need a Fixes tag:
Fixes: commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
thanks,
--
John Hubbard
NVIDIA
> arch/x86/include/asm/bootparam_utils.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/include/asm/bootparam_utils.h
> b/arch/x86/include/asm/bootparam_utils.h
> index 9e5f3c7..981fe92 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params
> *boot_params)
> BOOT_PARAM_PRESERVE(eddbuf_entries),
> BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> + BOOT_PARAM_PRESERVE(secure_boot),
> BOOT_PARAM_PRESERVE(hdr),
> BOOT_PARAM_PRESERVE(e820_table),
> BOOT_PARAM_PRESERVE(eddbuf),
>
From: "John S. Gruber" <[email protected]>
commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
else") now zeros the secure boot information passed by the boot loader or
by the kernel's efi handover mechanism. Include boot-params.secure_boot
in the preserve field list.
I noted a change in my computers between running signed 5.3-rc4 and 5.3-rc6
with signed kernels using the efi handoff protocol with grub. The kernel
log message "Secure boot enabled" becomes "Secure boot could not be
determined". The efi_main function in arch/x86/boot/compressed/eboot.c sets
this field early but it is subsequently zeroed by the above referenced
commit in the file arch/x86/include/asm/bootparam_utils.h
Fixes: commit a90118c445cc ("x86/boot: Save fields explicitly, zero
out everything else")
Signed-off-by: John S. Gruber <[email protected]>
---
Adjusted the patch for John Hubbard's comments.
arch/x86/include/asm/bootparam_utils.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/bootparam_utils.h
b/arch/x86/include/asm/bootparam_utils.h
index 9e5f3c7..981fe92 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params
*boot_params)
BOOT_PARAM_PRESERVE(eddbuf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(secure_boot),
BOOT_PARAM_PRESERVE(hdr),
BOOT_PARAM_PRESERVE(e820_table),
BOOT_PARAM_PRESERVE(eddbuf),
--
2.7.4
On Mon, Sep 02, 2019 at 12:00:54AM +0200, John S Gruber wrote:
> From: "John S. Gruber" <[email protected]>
>
> commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
> else") now zeros the secure boot information passed by the boot loader or
> by the kernel's efi handover mechanism. Include boot-params.secure_boot
> in the preserve field list.
>
> I noted a change in my computers between running signed 5.3-rc4 and 5.3-rc6
> with signed kernels using the efi handoff protocol with grub. The kernel
> log message "Secure boot enabled" becomes "Secure boot could not be
> determined". The efi_main function in arch/x86/boot/compressed/eboot.c sets
> this field early but it is subsequently zeroed by the above referenced
> commit in the file arch/x86/include/asm/bootparam_utils.h
>
> Fixes: commit a90118c445cc ("x86/boot: Save fields explicitly, zero
> out everything else")
> Signed-off-by: John S. Gruber <[email protected]>
> ---
>
> Adjusted the patch for John Hubbard's comments.
>
> arch/x86/include/asm/bootparam_utils.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/include/asm/bootparam_utils.h
> b/arch/x86/include/asm/bootparam_utils.h
> index 9e5f3c7..981fe92 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params
> *boot_params)
gmail has managed to chew this patch:
checking file arch/x86/include/asm/bootparam_utils.h
patch: **** malformed patch at line 48: *boot_params)
See: https://www.kernel.org/doc/html/latest/process/email-clients.html#gmail-web-gui
You might find a better client in there if you wanna send more patches
in the future.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 29d9a0b50736768f042752070e5cdf4e4d4c00df
Gitweb: https://git.kernel.org/tip/29d9a0b50736768f042752070e5cdf4e4d4c00df
Author: John S. Gruber <[email protected]>
AuthorDate: Mon, 02 Sep 2019 00:00:54 +02:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Mon, 02 Sep 2019 09:17:45 +02:00
x86/boot: Preserve boot_params.secure_boot from sanitizing
Commit
a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
now zeroes the secure boot setting information (enabled/disabled/...)
passed by the boot loader or by the kernel's EFI handover mechanism.
The problem manifests itself with signed kernels using the EFI handoff
protocol with grub and the kernel loses the information whether secure
boot is enabled in the firmware, i.e., the log message "Secure boot
enabled" becomes "Secure boot could not be determined".
efi_main() arch/x86/boot/compressed/eboot.c sets this field early but it
is subsequently zeroed by the above referenced commit.
Include boot_params.secure_boot in the preserve field list.
[ bp: restructure commit message and massage. ]
Fixes: a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
Signed-off-by: John S. Gruber <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: John Hubbard <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Juergen Gross <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: stable <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: x86-ml <[email protected]>
Link: https://lkml.kernel.org/r/CAPotdmSPExAuQcy9iAHqX3js_fc4mMLQOTr5RBGvizyCOPcTQQ@mail.gmail.com
---
arch/x86/include/asm/bootparam_utils.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 9e5f3c7..981fe92 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -70,6 +70,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
BOOT_PARAM_PRESERVE(eddbuf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+ BOOT_PARAM_PRESERVE(secure_boot),
BOOT_PARAM_PRESERVE(hdr),
BOOT_PARAM_PRESERVE(e820_table),
BOOT_PARAM_PRESERVE(eddbuf),