Message ID | 20150331164615.GA29388@morn.localdomain |
---|---|
State | New |
Headers | show |
>>> 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.pdf >> > 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
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;