Re: [PATCH] board: amlogic: fix buffler overflow in serial & usid read

On 20/03/2024 06:28, Dan Carpenter wrote:
> On Tue, Mar 19, 2024 at 03:53:24PM +0100, Neil Armstrong wrote:
>> While meson_sm_read_efuse() doesn't overflow, the string is not
>> zero terminated and env_set() will buffer overflow and add random
>> characters to environment.
>>
> 
> In the Linux kernel we would give this a CVE because it's information
> disclosure bug...

Yes probably

> 
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>>   board/amlogic/jethub-j80/jethub-j80.c | 6 ++++--
>>   board/amlogic/p200/p200.c             | 3 ++-
>>   board/amlogic/p201/p201.c             | 3 ++-
>>   board/amlogic/p212/p212.c             | 3 ++-
>>   board/amlogic/q200/q200.c             | 3 ++-
>>   5 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/board/amlogic/jethub-j80/jethub-j80.c b/board/amlogic/jethub-j80/jethub-j80.c
>> index 185880de13..d10492cc46 100644
>> --- a/board/amlogic/jethub-j80/jethub-j80.c
>> +++ b/board/amlogic/jethub-j80/jethub-j80.c
>> @@ -28,8 +28,8 @@
>>   int misc_init_r(void)
>>   {
>>   	u8 mac_addr[EFUSE_MAC_SIZE];
>             ^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This one is also a problem.  You can't pass non-terminated strings to
> eth_env_set_enetaddr().  We call strlen() on it in either hsearch_r() or
> env_get_from_linear().
> 
> All the other functions had a mac_addr[] issue as well.

Ack, I'll also fix those, I should have checked before...

> 
> Btw, this kind of bug is a good candidate for a static checker warning.
> I can create a Smatch check for this.  It would probably be easier in
> Coccinelle even, but I'm the Smatch maintainer.


Would be nice!

> 
> regards,
> dan carpenter
> 
> 
>> -	char serial[EFUSE_SN_SIZE];
>> -	char usid[EFUSE_USID_SIZE];
>> +	char serial[EFUSE_SN_SIZE + 1];
>> +	char usid[EFUSE_USID_SIZE + 1];
>>   	ssize_t len;
>>   	unsigned int adcval;
>>   	int ret;
>> @@ -46,6 +46,7 @@ int misc_init_r(void)
>>   	if (!env_get("serial")) {
>>   		len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial,
>>   					  EFUSE_SN_SIZE);
>> +		serial[len] = '\0';
>>   		if (len == EFUSE_SN_SIZE)
>>   			env_set("serial", serial);
> 

Thanks,
Neil