Re: [Qemu-devel] [SeaBIOS] [PATCH] smbios: ensure comparison SMBIOS string can't be paragraph aligned

[Bruce Rogers]

>>> On 3/31/2015 at 10:46 AM, Kevin O'Connor <address@hidden> 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 <address@hidden> 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