linux-kernel - Re: [PATCH v3 3/4] platform/x86: dell

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8a8ff0b4-49a9-4b07-abbd-243b754bfc0e@gmail.com>
Date: Fri, 30 May 2025 10:27:52 -0500
From: stuart hayes <stuart.w.hayes@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Hans de Goede <hdegoede@...hat.com>,
 platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v3 3/4] platform/x86: dell_rbu: Stop overwriting data
 buffer

On 5/30/2025 3:03 AM, Ilpo Järvinen wrote:
> On Thu, 29 May 2025, Stuart Hayes wrote:
> 
>> The dell_rbu driver will use memset() to clear the data held by each
>> packet when it is no longer needed (when the driver is unloaded, the
>> packet size is changed, etc).
>>
>> The amount of memory that is cleared (before this patch) is the normal
>> packet size. However, the last packet in the list may be smaller.
>>
>> Fix this to only clear the memory actually used by each packet, to prevent
>> it from writing past the end of data buffer.
>>
>> Signed-off-by: Stuart Hayes <stuart.w.hayes@...il.com>
> 
> This still doesn't have Fixes tag? If it writes part the buffer, there
> certainly should be one in this one. Did you perhaps add it to a wrong
> patch?
> 

This fixes a bug that's existed for the life of the driver. Should I use 
the patch that introduced the driver for "Fixes:"?

I don't think this has ever caused a problem, because, as far as I know, 
the Dell BIOS update program is the only user of this module, and it 
uses 4096 as the packet size. Because the packet data buffers are 
allocated with...

	ordernum = get_order(size);
	image_update_buffer =
		(unsigned char *)__get_free_pages(GRP_DMA32, ordernum);

...this will always allocate one full page for every packet data buffer.

I just happened to notice that the driver could overwrite the end of the 
buffer for the last packet if a packet size of more than 4096 was used, 
so I thought I'd fix that.

>> ---
>>   drivers/platform/x86/dell/dell_rbu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell_rbu.c b/drivers/platform/x86/dell/dell_rbu.c
>> index c03d4d55fcc1..7d5b26735a20 100644
>> --- a/drivers/platform/x86/dell/dell_rbu.c
>> +++ b/drivers/platform/x86/dell/dell_rbu.c
>> @@ -322,7 +322,7 @@ static void packet_empty_list(void)
>>   		 * zero out the RBU packet memory before freeing
>>   		 * to make sure there are no stale RBU packets left in memory
>>   		 */
>> -		memset(newpacket->data, 0, rbu_data.packetsize);
>> +		memset(newpacket->data, 0, newpacket->length);
>>   		set_memory_wb((unsigned long)newpacket->data,
>>   			1 << newpacket->ordernum);
>>   		free_pages((unsigned long) newpacket->data,
>>
>