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@gmail.com>
---
 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,
-- 
2.47.1

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@gmail.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?

> ---
>  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,
> 

-- 
 i.

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@gmail.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,
>>
> 

On Fri, 30 May 2025, stuart hayes wrote:

> 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@gmail.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:"?

Yes. They're normal commits just like every other commit. And since first 
commits are quite complicated, it's nothing unusual to find problems that 
have been there right from the beginning.

> 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.

It would be worth to mention enough these details then in the changelog so 
that other people such as stable maintainers can understand it so they can 
base their reasoning on facts provided rather than guesswork.

-- 
 i.


> > > ---
> > >   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,
> > > 
> > 
>