[PATCH] smbios: Fix table when no string is present
From: Matthias Brugger mbrugger@suse.com
When no string is present in a table, next_ptr points to the same location as eos. When calculating the string table length, we would only reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no strings a present.
Signed-off-by: Matthias Brugger mbrugger@suse.com
---
lib/smbios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/smbios.c b/lib/smbios.c index 7d463c84a9..d21d37cdac 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) { ctx->eos = eos; - ctx->next_ptr = eos; + ctx->next_ptr = eos + 1; ctx->last_str = NULL; }
Hi Matthias,
On Thu, 18 Mar 2021 at 00:30, matthias.bgg@kernel.org wrote:
From: Matthias Brugger mbrugger@suse.com
When no string is present in a table, next_ptr points to the same location as eos. When calculating the string table length, we would only reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no strings a present.
Signed-off-by: Matthias Brugger mbrugger@suse.com
lib/smbios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
There is a bug here but I don't think this is right fix. I remember worrying about this, making the same change as you did, reverting it and then forgetting about it :-( It has no tests.
You are effectively changing the definition of next_ptr here:
* @next_ptr: pointer to the start of the next string to be added. When the * table is not empty, this points to the byte after the \0 of the * previous string.
(there is a typo in that in the code)
I think that breaks adding new strings.
Can you instead change smbios_string_table_len() to add 2?
diff --git a/lib/smbios.c b/lib/smbios.c index 7d463c84a9..d21d37cdac 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) { ctx->eos = eos;
ctx->next_ptr = eos;
ctx->next_ptr = eos + 1; ctx->last_str = NULL;}
-- 2.30.2
Regards, Simon
On 25/03/2021 01:38, Simon Glass wrote:
Hi Matthias,
On Thu, 18 Mar 2021 at 00:30, matthias.bgg@kernel.org wrote:
From: Matthias Brugger mbrugger@suse.com
When no string is present in a table, next_ptr points to the same location as eos. When calculating the string table length, we would only reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no strings a present.
Signed-off-by: Matthias Brugger mbrugger@suse.com
lib/smbios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
There is a bug here but I don't think this is right fix. I remember worrying about this, making the same change as you did, reverting it and then forgetting about it :-( It has no tests.
You are effectively changing the definition of next_ptr here:
- @next_ptr: pointer to the start of the next string to be added. When the
- table is not empty, this points to the byte after the \0 of the
- previous string.
That's true.
(there is a typo in that in the code)
I think that breaks adding new strings.
Well it doesn't because when adding a new string, ctx->next_ptr gets overwritten [1]. It is only used to calculate the lenght of the string in smbios_string_table_len() to calculate the size of the table [2]. But yes it's not the obvious way to handle the case when no string is present.
Can you instead change smbios_string_table_len() to add 2?
Do you mean returning 2 when no string is present (ctx->next_ptr == ctx->eos)?
Adding 2 will break the case when we have a string present, as a string already finishes with '\0' and we only need a second '\0'.
Regards, Matthias
[1] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L95 [2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L196
diff --git a/lib/smbios.c b/lib/smbios.c index 7d463c84a9..d21d37cdac 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) { ctx->eos = eos;
ctx->next_ptr = eos;
ctx->next_ptr = eos + 1; ctx->last_str = NULL;}
-- 2.30.2
Regards, Simon
Hi Matthias,
On Thu, 25 Mar 2021 at 16:38, Matthias Brugger matthias.bgg@gmail.com wrote:
On 25/03/2021 01:38, Simon Glass wrote:
Hi Matthias,
On Thu, 18 Mar 2021 at 00:30, matthias.bgg@kernel.org wrote:
From: Matthias Brugger mbrugger@suse.com
When no string is present in a table, next_ptr points to the same location as eos. When calculating the string table length, we would only reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no strings a present.
Signed-off-by: Matthias Brugger mbrugger@suse.com
lib/smbios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
There is a bug here but I don't think this is right fix. I remember worrying about this, making the same change as you did, reverting it and then forgetting about it :-( It has no tests.
You are effectively changing the definition of next_ptr here:
- @next_ptr: pointer to the start of the next string to be added. When the
- table is not empty, this points to the byte after the \0 of the
- previous string.
That's true.
(there is a typo in that in the code)
I think that breaks adding new strings.
Well it doesn't because when adding a new string, ctx->next_ptr gets overwritten [1]. It is only used to calculate the lenght of the string in smbios_string_table_len() to calculate the size of the table [2]. But yes it's not the obvious way to handle the case when no string is present.
Can you instead change smbios_string_table_len() to add 2?
Do you mean returning 2 when no string is present (ctx->next_ptr == ctx->eos)?
Adding 2 will break the case when we have a string present, as a string already finishes with '\0' and we only need a second '\0'.
Oh dear, this is complicated. I think we need to add a few unit tests. Do you have time to try that?
Regards, Simon
Regards, Matthias
[1] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L95 [2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L196
diff --git a/lib/smbios.c b/lib/smbios.c index 7d463c84a9..d21d37cdac 100644 --- a/lib/smbios.c +++ b/lib/smbios.c @@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop) static void smbios_set_eos(struct smbios_ctx *ctx, char *eos) { ctx->eos = eos;
ctx->next_ptr = eos;
ctx->next_ptr = eos + 1; ctx->last_str = NULL;}
-- 2.30.2
Regards, Simon
participants (3)
-
Matthias Brugger -
matthias.bgg＠kernel.org -
Simon Glass