The SMBIOS anchor string _SM_ is stored within SeaBIOS to validate an SMBIOS entry point structure. There is the possibility (observed) that this comparison string ends up paragraph aligned and mistakenly found during a search for the real SMBIOS entry point. Ensure it will never end up on a paragraph boundary by storing it at odd alignment.
Signed-off-by: Bruce Rogers brogers@suse.com --- src/fw/biostables.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/fw/biostables.c b/src/fw/biostables.c index 50a891b..b8ff86f 100644 --- a/src/fw/biostables.c +++ b/src/fw/biostables.c @@ -18,6 +18,9 @@ #include "util.h" // copy_table #include "x86.h" // outb
+// ensure signature cannot be found on paragraph boundary +const char smbios_sig_str[] __aligned(2) VARFSEG = " _SM_"; + struct pir_header *PirAddr VARFSEG;
void @@ -271,7 +274,7 @@ copy_smbios(void *pos) if (SMBiosAddr) return; struct smbios_entry_point *p = pos; - if (memcmp(p->anchor_string, "_SM_", 4)) + if (memcmp(p->anchor_string, smbios_sig_str + 1, 4)) return; if (checksum(pos, 0x10) != 0) return;
On Mon, Mar 30, 2015 at 05:06:30PM -0600, Bruce Rogers wrote:
The SMBIOS anchor string _SM_ is stored within SeaBIOS to validate an SMBIOS entry point structure. There is the possibility (observed) that this comparison string ends up paragraph aligned and mistakenly found during a search for the real SMBIOS entry point. Ensure it will never end up on a paragraph boundary by storing it at odd alignment.
Thanks.
What OS was this on? It's really an OS bug as the OS needs to check both the signature and the checksum.
My preferred approach to addressing this would be to turn p->anchor_string into a u32 and do an integer compare instead of a string compare. Although technically this can lead to the same potential issue, in practice it should not happen because SeaBIOS' init code is relocated out of the f-segment during startup (while static strings are generally not).
-Kevin
On 3/30/2015 at 10:02 PM, Kevin O'Connor kevin@koconnor.net wrote:
On Mon, Mar 30, 2015 at 05:06:30PM -0600, Bruce Rogers wrote:
The SMBIOS anchor string _SM_ is stored within SeaBIOS to validate an SMBIOS entry point structure. There is the possibility (observed) that this comparison string ends up paragraph aligned and mistakenly found during a search for the real SMBIOS entry point. Ensure it will never end up on a paragraph boundary by storing it at odd alignment.
Thanks.
What OS was this on? It's really an OS bug as the OS needs to check both the signature and the checksum.
My preferred approach to addressing this would be to turn p->anchor_string into a u32 and do an integer compare instead of a string compare. Although technically this can lead to the same potential issue, in practice it should not happen because SeaBIOS' init code is relocated out of the f-segment during startup (while static strings are generally not).
-Kevin
This was actually flagged by the QEMU make check test in tests/bios-tables-test.c, where the code has asserts based on the first find of the _SM_ string found on a paragraph boundary in the f segment. It assumes that the string found is sufficient to identify the smbios entry point structure.
I read in http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.0.0.pd... where there are number of steps needed to verify the entry-point, beyond finding the anchor string. So I assume that the make check test needs to be fixed.
But I wonder if bios creators are also supposed to ensure that there is no such string findable on a paragraph boundary so as to avoid any potential confusion? I don't have expereince writing bios's so I am only guessing at that.
Bruce
On Tue, Mar 31, 2015 at 08:49:58AM -0600, Bruce Rogers wrote:
On 3/30/2015 at 10:02 PM, Kevin O'Connor kevin@koconnor.net wrote:
On Mon, Mar 30, 2015 at 05:06:30PM -0600, Bruce Rogers wrote:
The SMBIOS anchor string _SM_ is stored within SeaBIOS to validate an SMBIOS entry point structure. There is the possibility (observed) that this comparison string ends up paragraph aligned and mistakenly found during a search for the real SMBIOS entry point. Ensure it will never end up on a paragraph boundary by storing it at odd alignment.
Thanks.
What OS was this on? It's really an OS bug as the OS needs to check both the signature and the checksum.
My preferred approach to addressing this would be to turn p->anchor_string into a u32 and do an integer compare instead of a string compare. Although technically this can lead to the same potential issue, in practice it should not happen because SeaBIOS' init code is relocated out of the f-segment during startup (while static strings are generally not).
-Kevin
This was actually flagged by the QEMU make check test in tests/bios-tables-test.c, where the code has asserts based on the first find of the _SM_ string found on a paragraph boundary in the f segment. It assumes that the string found is sufficient to identify the smbios entry point structure.
I read in http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.0.0.pd... where there are number of steps needed to verify the entry-point, beyond finding the anchor string. So I assume that the make check test needs to be fixed.
But I wonder if bios creators are also supposed to ensure that there is no such string findable on a paragraph boundary so as to avoid any potential confusion? I don't have expereince writing bios's so I am only guessing at that.
I guess on physical hardware that would be mitigated by the BIOS's .rodata (or whatever the equivalent thing is called in BIOS-speak) NOT falling within 0xf0000..0xfffff :)
If there's no way to guarantee that with SeaBIOS, I suppose the qemu test could be rewritten to account for the possibility of "false positive" signatures...
--Gabriel
On Tue, Mar 31, 2015 at 12:37:30PM -0400, Gabriel L. Somlo wrote:
On Tue, Mar 31, 2015 at 08:49:58AM -0600, Bruce Rogers wrote:
On 3/30/2015 at 10:02 PM, Kevin O'Connor kevin@koconnor.net wrote:
On Mon, Mar 30, 2015 at 05:06:30PM -0600, Bruce Rogers wrote:
The SMBIOS anchor string _SM_ is stored within SeaBIOS to validate an SMBIOS entry point structure. There is the possibility (observed) that this comparison string ends up paragraph aligned and mistakenly found during a search for the real SMBIOS entry point. Ensure it will never end up on a paragraph boundary by storing it at odd alignment.
Thanks.
What OS was this on? It's really an OS bug as the OS needs to check both the signature and the checksum.
My preferred approach to addressing this would be to turn p->anchor_string into a u32 and do an integer compare instead of a string compare. Although technically this can lead to the same potential issue, in practice it should not happen because SeaBIOS' init code is relocated out of the f-segment during startup (while static strings are generally not).
-Kevin
This was actually flagged by the QEMU make check test in tests/bios-tables-test.c, where the code has asserts based on the first find of the _SM_ string found on a paragraph boundary in the f segment. It assumes that the string found is sufficient to identify the smbios entry point structure.
I read in http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.0.0.pd... where there are number of steps needed to verify the entry-point, beyond finding the anchor string. So I assume that the make check test needs to be fixed.
But I wonder if bios creators are also supposed to ensure that there is no such string findable on a paragraph boundary so as to avoid any potential confusion? I don't have expereince writing bios's so I am only guessing at that.
I guess on physical hardware that would be mitigated by the BIOS's .rodata (or whatever the equivalent thing is called in BIOS-speak) NOT falling within 0xf0000..0xfffff :)
If there's no way to guarantee that with SeaBIOS, I suppose the qemu test could be rewritten to account for the possibility of "false positive" signatures...
I would do both - change SeaBIOS to (in practice) not put "_SM_" in the f-segment and change the QEMU test to not stop on the first "_SM_" it finds. SeaBIOS patch below (untested).
-Kevin
diff --git a/src/fw/biostables.c b/src/fw/biostables.c index 50a891b..450aca2 100644 --- a/src/fw/biostables.c +++ b/src/fw/biostables.c @@ -271,7 +271,7 @@ copy_smbios(void *pos) if (SMBiosAddr) return; struct smbios_entry_point *p = pos; - if (memcmp(p->anchor_string, "_SM_", 4)) + if (p->signature != SMBIOS_SIGNATURE) return; if (checksum(pos, 0x10) != 0) return; diff --git a/src/fw/smbios.c b/src/fw/smbios.c index dba0541..f3b5ad9 100644 --- a/src/fw/smbios.c +++ b/src/fw/smbios.c @@ -37,7 +37,7 @@ smbios_entry_point_setup(u16 max_structure_size,
struct smbios_entry_point ep; memset(&ep, 0, sizeof(ep)); - memcpy(ep.anchor_string, "_SM_", 4); + ep.signature = SMBIOS_SIGNATURE; ep.length = 0x1f; ep.smbios_major_version = 2; ep.smbios_minor_version = 4; diff --git a/src/std/smbios.h b/src/std/smbios.h index 0513716..4ccf2ea 100644 --- a/src/std/smbios.h +++ b/src/std/smbios.h @@ -3,11 +3,13 @@
#include "types.h" // u32
+#define SMBIOS_SIGNATURE 0x5f4d535f // "_SM_" + /* SMBIOS entry point -- must be written to a 16-bit aligned address between 0xf0000 and 0xfffff. */ struct smbios_entry_point { - char anchor_string[4]; + u32 signature; u8 checksum; u8 length; u8 smbios_major_version;
On 3/31/2015 at 10:46 AM, Kevin O'Connor kevin@koconnor.net wrote:
On Tue, Mar 31, 2015 at 12:37:30PM -0400, Gabriel L. Somlo wrote:
On Tue, Mar 31, 2015 at 08:49:58AM -0600, Bruce Rogers wrote:
On 3/30/2015 at 10:02 PM, Kevin O'Connor kevin@koconnor.net wrote:
On Mon, Mar 30, 2015 at 05:06:30PM -0600, Bruce Rogers wrote:
The SMBIOS anchor string _SM_ is stored within SeaBIOS to validate an SMBIOS entry point structure. There is the possibility (observed) that this comparison string ends up paragraph aligned and mistakenly found during a search for the real SMBIOS entry point. Ensure it will never end up on a paragraph boundary by storing it at odd alignment.
Thanks.
What OS was this on? It's really an OS bug as the OS needs to check both the signature and the checksum.
My preferred approach to addressing this would be to turn p->anchor_string into a u32 and do an integer compare instead of a string compare. Although technically this can lead to the same potential issue, in practice it should not happen because SeaBIOS' init code is relocated out of the f-segment during startup (while static strings are generally not).
-Kevin
This was actually flagged by the QEMU make check test in tests/bios-tables-test.c, where the code has asserts based on the first find of the _SM_ string found on a paragraph boundary in the f segment. It assumes that the string found is sufficient to identify the smbios entry point structure.
I read in
http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.0.0.pd...
where there are number of steps needed to verify the entry-point, beyond finding the anchor string. So I assume that the make check test needs to be fixed.
But I wonder if bios creators are also supposed to ensure that there is no such string findable on a paragraph boundary so as to avoid any potential confusion? I don't have expereince writing bios's so I am only guessing at that.
I guess on physical hardware that would be mitigated by the BIOS's .rodata (or whatever the equivalent thing is called in BIOS-speak) NOT falling within 0xf0000..0xfffff :)
If there's no way to guarantee that with SeaBIOS, I suppose the qemu test could be rewritten to account for the possibility of "false positive" signatures...
I would do both - change SeaBIOS to (in practice) not put "_SM_" in the f-segment and change the QEMU test to not stop on the first "_SM_" it finds. SeaBIOS patch below (untested).
-Kevin
diff --git a/src/fw/biostables.c b/src/fw/biostables.c index 50a891b..450aca2 100644 --- a/src/fw/biostables.c +++ b/src/fw/biostables.c @@ -271,7 +271,7 @@ copy_smbios(void *pos) if (SMBiosAddr) return; struct smbios_entry_point *p = pos;
- if (memcmp(p->anchor_string, "_SM_", 4))
- if (p->signature != SMBIOS_SIGNATURE) return; if (checksum(pos, 0x10) != 0) return;
diff --git a/src/fw/smbios.c b/src/fw/smbios.c index dba0541..f3b5ad9 100644 --- a/src/fw/smbios.c +++ b/src/fw/smbios.c @@ -37,7 +37,7 @@ smbios_entry_point_setup(u16 max_structure_size,
struct smbios_entry_point ep; memset(&ep, 0, sizeof(ep));
- memcpy(ep.anchor_string, "_SM_", 4);
- ep.signature = SMBIOS_SIGNATURE; ep.length = 0x1f; ep.smbios_major_version = 2; ep.smbios_minor_version = 4;
diff --git a/src/std/smbios.h b/src/std/smbios.h index 0513716..4ccf2ea 100644 --- a/src/std/smbios.h +++ b/src/std/smbios.h @@ -3,11 +3,13 @@
#include "types.h" // u32
+#define SMBIOS_SIGNATURE 0x5f4d535f // "_SM_"
/* SMBIOS entry point -- must be written to a 16-bit aligned address between 0xf0000 and 0xfffff. */ struct smbios_entry_point {
- char anchor_string[4];
- u32 signature; u8 checksum; u8 length; u8 smbios_major_version;
Agreed. Thanks.
Bruce
-
Bruce Rogers -
Gabriel L. Somlo -
Kevin O'Connor