[SeaBIOS][PATCH] smbios: Add missing zero byte to Type 0

[Sam Eiderman]

According to SMBIOS Specification, section 6.1.3 Text Strings:
"Text strings associated with a given SMBIOS structure are returned in
the dmiStructBuffer, appended directly after the formatted portion of the
structure. This method of returning string information eliminates the
need for application software to deal with pointers embedded in the
SMBIOS structure. Each string is terminated with a null (00h) BYTE and
the set of strings is terminated with an additional null (00h) BYTE”

Furthermore:
"If the formatted portion of the structure contains string-reference
fields and all the string fields are set to 0 (no string references),
the formatted section of the structure is followed by two null (00h)
BYTES"

From the above it can be seen that any SMBIOS type which contains string
references should end with an additional zero byte.

This is currently handled in all SMBIOS types which use
load_str_field_with_default() besides type0.
Therefore, add the missing zero byte to SMBIOS Type 0.

Running QEMU with:
    -machine pc-i440fx-2.0 (for legacy smbios)
    -smbios type=0,vendor=,version=,date= (for zero str_index)
Will cause SMBIOS type0 entry to overrun type1 entry.

Reviewed-by: Ross Philipson 
Reviewed-By: Liran Alon 
Signed-off-by: Sam Eiderman 
---
 src/fw/smbios.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/fw/smbios.c b/src/fw/smbios.c
index f3b5ad9d..62a563b2 100644
--- a/src/fw/smbios.c
+++ b/src/fw/smbios.c
@@ -205,6 +205,10 @@ smbios_init_type_0(void *start)

*end = 0;
     end++;
+    if (!str_index) {
+        *end = 0;
+        end++;
+    }

return end;
 }

[Sam]

If possible also add:

Reviewed-by: Mark Kanda <[e-mail] >

Thanks,
Sam
> On 24 Apr 2019, at 17:04, Sam Eiderman  wrote:
>
> According to SMBIOS Specification, section 6.1.3 Text Strings:
> "Text strings associated with a given SMBIOS structure are returned in
> the dmiStructBuffer, appended directly after the formatted portion of the
> structure. This method of returning string information eliminates the
> need for application software to deal with pointers embedded in the
> SMBIOS structure. Each string is terminated with a null (00h) BYTE and
> the set of strings is terminated with an additional null (00h) BYTE”
>
> Furthermore:
> "If the formatted portion of the structure contains string-reference
> fields and all the string fields are set to 0 (no string references),
> the formatted section of the structure is followed by two null (00h)
> BYTES"
>
> From the above it can be seen that any SMBIOS type which contains string
> references should end with an additional zero byte.
>
> This is currently handled in all SMBIOS types which use
> load_str_field_with_default() besides type0.
> Therefore, add the missing zero byte to SMBIOS Type 0.
>
> Running QEMU with:
>    -machine pc-i440fx-2.0 (for legacy smbios)
>    -smbios type=0,vendor=,version=,date= (for zero str_index)
> Will cause SMBIOS type0 entry to overrun type1 entry.
>
> Reviewed-by: Ross Philipson 
> Reviewed-By: Liran Alon 
> Signed-off-by: Sam Eiderman
>
> src/fw/smbios.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/fw/smbios.c b/src/fw/smbios.c
> index f3b5ad9d..62a563b2 100644
> --- a/src/fw/smbios.c
> +++ b/src/fw/smbios.c
> @@ -205,6 +205,10 @@ smbios_init_type_0(void *start)
> *end = 0;
> end++;
>
> if (!str_index) {
>    *end = 0;
>    end++;
>
> }
>
> return end;
> }
>
> 2.13.3

[Kevin O'Connor]

On Wed, Apr 24, 2019 at 05:04:09PM +0300, Sam Eiderman wrote:
> According to SMBIOS Specification, section 6.1.3 Text Strings:
> "Text strings associated with a given SMBIOS structure are returned in
> the dmiStructBuffer, appended directly after the formatted portion of the
> structure. This method of returning string information eliminates the
> need for application software to deal with pointers embedded in the
> SMBIOS structure. Each string is terminated with a null (00h) BYTE and
> the set of strings is terminated with an additional null (00h) BYTE”
>
> Furthermore:
> "If the formatted portion of the structure contains string-reference
> fields and all the string fields are set to 0 (no string references),
> the formatted section of the structure is followed by two null (00h)
> BYTES"
>
> From the above it can be seen that any SMBIOS type which contains string
> references should end with an additional zero byte.
>
> This is currently handled in all SMBIOS types which use
> load_str_field_with_default() besides type0.
> Therefore, add the missing zero byte to SMBIOS Type 0.
>
> Running QEMU with:
>     -machine pc-i440fx-2.0 (for legacy smbios)
>     -smbios type=0,vendor=,version=,date= (for zero str_index)
> Will cause SMBIOS type0 entry to overrun type1 entry.

Okay, thanks.  If I understand correctly - this only impacts
situations where the user manually sets vender, version, and date to
null strings?  If so, I don't see a harm in fixing it in SeaBIOS.

Gerd - do you agree?

-Kevin

[Gerd Hoffmann]

Hi,
> > Running QEMU with:
> >     -machine pc-i440fx-2.0 (for legacy smbios)
> >     -smbios type=0,vendor=,version=,date= (for zero str_index)
> > Will cause SMBIOS type0 entry to overrun type1 entry.
>
> Okay, thanks.  If I understand correctly - this only impacts
> situations where the user manually sets vender, version, and date to
> null strings?  If so, I don't see a harm in fixing it in SeaBIOS.
>
> Gerd - do you agree?

Yes.

cheers,
  Gerd

[Kevin O'Connor]

On Wed, Apr 24, 2019 at 05:04:09PM +0300, Sam Eiderman wrote:
> From the above it can be seen that any SMBIOS type which contains string
> references should end with an additional zero byte.

Thanks.  I committed this change.

I noticed during the commit that this change was made to the legacy
smbios tables.  Just to be clear, we recommend that the "machine
implementation" (eg, coreboot, qemu, xen, etc.) pass appropriate
smbios tables to SeaBIOS (thus bypassing the legacy smbios generation
code).  See the code in src/fw/biostables.c for reference.

-Kevin

[Sam]

Yes we were aware of that, we do not wish to change legacy SMBIOS code behavior and we are aware of the “machine implementation” technique for both legacy and non-legacy SMBIOS tables.
This commit was simply a bug fix.

Thanks for applying it!
Sam
> On 7 May 2019, at 17:45, Kevin O'Connor  wrote:
>
> On Wed, Apr 24, 2019 at 05:04:09PM +0300, Sam Eiderman wrote:
> > From the above it can be seen that any SMBIOS type which contains string
> > references should end with an additional zero byte.
>
> Thanks.  I committed this change.
>
> I noticed during the commit that this change was made to the legacy
> smbios tables.  Just to be clear, we recommend that the "machine
> implementation" (eg, coreboot, qemu, xen, etc.) pass appropriate
> smbios tables to SeaBIOS (thus bypassing the legacy smbios generation
> code).  See the code in src/fw/biostables.c for reference.
>
> -Kevin