x86/geode/alix: bound the BIOS name copy to the scanned window

[PATCH] x86/geode/alix: bound the BIOS name copy to the scanned window

Posted by Pengpeng Hou 2 months, 1 week ago

alix_present() scans the BIOS window one byte at a time looking for
either "PC Engines ALIX." or "PC Engines\0ALIX.". The scan
limit only ensures that the signature and the trailing board digit fit
in the remaining BIOS mapping, but after a match the code copies 64
bytes from the current pointer into a fixed local name buffer.

If the signature is found near the end of the mapped BIOS region,
memcpy(name, p, sizeof(name)) reads past the end of the scan window. The
copied bytes are then searched with strchr(), so the local buffer should
also be NUL-terminated explicitly.

Copy only the bytes that remain in the mapped BIOS region and terminate
the local buffer before using string helpers.

Fixes: d4f3e350172a ("x86: geode: New PCEngines Alix system driver")
Cc: stable@vger.kernel.org
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 arch/x86/platform/geode/alix.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c
index be65cd704e21..e01a607fa3b5 100644
--- a/arch/x86/platform/geode/alix.c
+++ b/arch/x86/platform/geode/alix.c
@@ -72,11 +72,20 @@ static bool __init alix_present(unsigned long bios_phys,
 	for (p = bios_virt; p < scan_end; p++) {
 		const char *tail;
 		char *a;
+		size_t copy_len;
 
 		if (memcmp(p, alix_sig, alix_sig_len) != 0)
 			continue;
 
-		memcpy(name, p, sizeof(name));
+		/*
+		 * The scan window only proves that the signature and the
+		 * trailing board digit fit in the mapped BIOS region.
+		 */
+		copy_len = min_t(size_t, sizeof(name) - 1,
+				 bios_virt + bios_len - p);
+
+		memcpy(name, p, copy_len);
+		name[copy_len] = '\0';
 
 		/* remove the first \0 character from string */
 		a = strchr(name, '\0');
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] x86/geode/alix: bound the BIOS name copy to the scanned window

Posted by Borislav Petkov 1 week, 4 days ago

On Thu, Apr 02, 2026 at 09:42:26PM +0800, Pengpeng Hou wrote:
> alix_present() scans the BIOS window one byte at a time looking for
> either "PC Engines ALIX." or "PC Engines\0ALIX.". The scan
> limit only ensures that the signature and the trailing board digit fit
> in the remaining BIOS mapping, but after a match the code copies 64
> bytes from the current pointer into a fixed local name buffer.
> 
> If the signature is found near the end of the mapped BIOS region,
> memcpy(name, p, sizeof(name)) reads past the end of the scan window. The
> copied bytes are then searched with strchr(), so the local buffer should
> also be NUL-terminated explicitly.
> 
> Copy only the bytes that remain in the mapped BIOS region and terminate
> the local buffer before using string helpers.
> 
> Fixes: d4f3e350172a ("x86: geode: New PCEngines Alix system driver")

This patch is from 2011.

> Cc: stable@vger.kernel.org
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  arch/x86/platform/geode/alix.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c
> index be65cd704e21..e01a607fa3b5 100644
> --- a/arch/x86/platform/geode/alix.c
> +++ b/arch/x86/platform/geode/alix.c
> @@ -72,11 +72,20 @@ static bool __init alix_present(unsigned long bios_phys,
>  	for (p = bios_virt; p < scan_end; p++) {
>  		const char *tail;
>  		char *a;
> +		size_t copy_len;
>  
>  		if (memcmp(p, alix_sig, alix_sig_len) != 0)
>  			continue;
>  
> -		memcpy(name, p, sizeof(name));
> +		/*
> +		 * The scan window only proves that the signature and the
> +		 * trailing board digit fit in the mapped BIOS region.
> +		 */
> +		copy_len = min_t(size_t, sizeof(name) - 1,
> +				 bios_virt + bios_len - p);
> +
> +		memcpy(name, p, copy_len);
> +		name[copy_len] = '\0';
>  
>  		/* remove the first \0 character from string */
>  		a = strchr(name, '\0');
> -- 
> 2.50.1 (Apple Git-155)

Are you saying you have this Geode thing? And this patch is fixing *something*
on it?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/geode/alix: bound the BIOS name copy to the scanned window

Posted by Ed Wildgoose 1 week, 3 days ago

These are boards used in the PCEngines Alix boards. They are/were super router boards. Used the same
chip as in the "One Laptop per child".

They are somewhere between 486 and 586 in capabilities. Right at the edge of what people would
likely want today and not been sold for many years. However, they are/were amazing and I still have
many customers using them daily.

I didn't try the fix, but the concept seems good. However, I would think that all these devices
received their last bios update years back... My guess is this is an AI patch discovery? However, I
guess the sooner we get these in the less they will get brought up by the next person using the same
tools? (Apologies if I'm belittling some hard work here? Don't mean it in a mean way?)

Ed W

On 03/06/2026 06:21, Borislav Petkov wrote:
> On Thu, Apr 02, 2026 at 09:42:26PM +0800, Pengpeng Hou wrote:
>> alix_present() scans the BIOS window one byte at a time looking for
>> either "PC Engines ALIX." or "PC Engines\0ALIX.". The scan
>> limit only ensures that the signature and the trailing board digit fit
>> in the remaining BIOS mapping, but after a match the code copies 64
>> bytes from the current pointer into a fixed local name buffer.
>>
>> If the signature is found near the end of the mapped BIOS region,
>> memcpy(name, p, sizeof(name)) reads past the end of the scan window. The
>> copied bytes are then searched with strchr(), so the local buffer should
>> also be NUL-terminated explicitly.
>>
>> Copy only the bytes that remain in the mapped BIOS region and terminate
>> the local buffer before using string helpers.
>>
>> Fixes: d4f3e350172a ("x86: geode: New PCEngines Alix system driver")
> This patch is from 2011.
>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
>> ---
>>  arch/x86/platform/geode/alix.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c
>> index be65cd704e21..e01a607fa3b5 100644
>> --- a/arch/x86/platform/geode/alix.c
>> +++ b/arch/x86/platform/geode/alix.c
>> @@ -72,11 +72,20 @@ static bool __init alix_present(unsigned long bios_phys,
>>  	for (p = bios_virt; p < scan_end; p++) {
>>  		const char *tail;
>>  		char *a;
>> +		size_t copy_len;
>>  
>>  		if (memcmp(p, alix_sig, alix_sig_len) != 0)
>>  			continue;
>>  
>> -		memcpy(name, p, sizeof(name));
>> +		/*
>> +		 * The scan window only proves that the signature and the
>> +		 * trailing board digit fit in the mapped BIOS region.
>> +		 */
>> +		copy_len = min_t(size_t, sizeof(name) - 1,
>> +				 bios_virt + bios_len - p);
>> +
>> +		memcpy(name, p, copy_len);
>> +		name[copy_len] = '\0';
>>  
>>  		/* remove the first \0 character from string */
>>  		a = strchr(name, '\0');
>> -- 
>> 2.50.1 (Apple Git-155)
> Are you saying you have this Geode thing? And this patch is fixing *something*
> on it?
>

Re: [PATCH] x86/geode/alix: bound the BIOS name copy to the scanned window

Posted by Borislav Petkov 1 week, 3 days ago

Hi Ed,

On Wed, Jun 03, 2026 at 11:44:41AM +0100, Ed Wildgoose wrote:

please do not top-post when replying to a public thread.

> These are boards used in the PCEngines Alix boards. They are/were super
> router boards. Used the same chip as in the "One Laptop per child".

Right, I suspected something like that. And also that none of us has this hw
for testing... :-\

> They are somewhere between 486 and 586 in capabilities. Right at the edge of
> what people would likely want today and not been sold for many years.
> However, they are/were amazing and I still have many customers using them
> daily.

Are you saying, you'd be willing to test patches? :-P

> I didn't try the fix, but the concept seems good. However, I would think
> that all these devices received their last bios update years back... My
> guess is this is an AI patch discovery?

Likely. This is why I'm playing stupid and asking questions. :-)

> However, I guess the sooner we get these in the less they will get brought
> up by the next person using the same tools?

As long as someone can test them and tell me they don't break anything.
Because I can't fix anything from here without the hw and I am not even
willing to try to fix such old stuff. We have enough bugs as it is.

> (Apologies if I'm belittling some hard work here? Don't mean it in a mean
> way?)

Nothing to worry about - I appreciate any correction and constructive feedback
I can get. So no worries.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette