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: <054f2f19-cb2e-7633-8976-96c2fac7b746@linux.intel.com>
Date: Fri, 30 May 2025 18:46:06 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: stuart hayes <stuart.w.hayes@...il.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 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@...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:"?

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