Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 25 Jul 2019 09:22:10 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     hpa@zytor.com
cc:     john.hubbard@gmail.com, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>, x86@kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        John Hubbard <jhubbard@nvidia.com>
Subject: Re: [PATCH 1/1] x86/boot: clear some fields explicitly
In-Reply-To: <B7DC31CA-E378-445A-A937-1B99490C77B4@zytor.com>
Message-ID: <alpine.DEB.2.21.1907250848050.1791@nanos.tec.linutronix.de>
References: <20190724231528.32381-1-jhubbard@nvidia.com> <20190724231528.32381-2-jhubbard@nvidia.com> <B7DC31CA-E378-445A-A937-1B99490C77B4@zytor.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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);
 	}
 }