linux-kernel - Re: [PATCH] staging: fbtft: fix potential memory leak in fbtft_framebuffer

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: <aF3CwnHyW5HHzDSG@surfacebook.localdomain>
Date: Fri, 27 Jun 2025 00:59:30 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Andy Shevchenko <andriy.shevchenko@...el.com>,
	Abdun Nihaal <abdun.nihaal@...il.com>, andy@...nel.org,
	gregkh@...uxfoundation.org, lorenzo.stoakes@...cle.com,
	tzimmermann@...e.de, riyandhiman14@...il.com, willy@...radead.org,
	notro@...nnes.org, thomas.petazzoni@...e-electrons.com,
	dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: fbtft: fix potential memory leak in
 fbtft_framebuffer_alloc()

Thu, Jun 26, 2025 at 11:11:39PM +0300, Dan Carpenter kirjoitti:
> On Thu, Jun 26, 2025 at 08:50:43PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 26, 2025 at 10:54:10PM +0530, Abdun Nihaal wrote:

...

> > >  release_framebuf:
> > > +	fb_deferred_io_cleanup(info);
> > >  	framebuffer_release(info);
> > 
> > While the fix sounds good, there are still problems in the driver in this area:
> > 
> > 1) managed resources allocation is mixed up with plain allocations
> > (as you discovery hints);
> > 
> > 2) the order in fbtft_framebuffer_release() is asymmetrical to what
> > we have in fbtft_framebuffer_alloc().
> > 
> > I would recommend to study this code a bit more and provide the following
> > patches as a result:
> > 
> > 1) fixing the order in fbtft_framebuffer_release();
> > 
> > 2) moving vmem allocation closer to when it's needed, i.e. just after
> > successful allocation of the info; at the same time move txbuf allocation
> > from managed to unmanaged (drop devm, add respective kfree() calls where
> > it's required);
> 
> Symetrical in this sense means that the cleanup in
> fbtft_framebuffer_release() and in fbtft_framebuffer_alloc() are
> similar:
> 
> 	fb_deferred_io_cleanup();
> 	vfree();
>  	framebuffer_release();
> 
> I feel like number 1 and 2 are sort of opposite approaches to making the
> order symmetrical.  #1 is changing fbtft_framebuffer_release() and #2 is
> changing fbtft_framebuffer_alloc().  #2 is the less awkward approach.
> 
> > 3) this patch.
> > 
> > All three should have the respective Fixes tags and hence may be backported.
> 
> Changing the order isn't a bug fix so it wouldn't get a Fixes tag.
> I agree with Andy that the code isn't beautiful.  But I think it's
> easier to just fix the bug, and do the cleanup later as an optional
> patch 2/2.  I would also have been fine with a larger patch that does
> the cleanup and the bug fix in one patch but I think other people
> won't like that.

Ah, you have a point. Yes, the moving vmem allocation will solve the ordering
issue.

-- 
With Best Regards,
Andy Shevchenko