linux-kernel - Re: iio, syfs, devres: devm_kmalloc not aligned to pow2 size argument

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] [day] [month] [year] [list]

Message-ID: <b9b7582409247dc088ea2df64af24024@gmail.com>
Date: Thu, 14 Nov 2024 17:09:58 +0100
From: Matteo Martelli <matteomartelli3@...il.com>
To: Zijun Hu <zijun_hu@...oud.com>
Cc: Marc Gonzalez <marc.w.gonzalez@...e.fr>, Peter Rosin <peda@...ntia.se>,
	"Rafael J. Wysocki" <rafael@...nel.org>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, Jonathan Cameron <jic23@...nel.org>,
	Joe Perches <joe@...ches.com>, Jens Axboe <axboe@...nel.dk>, Peter Zijlstra
	<peterz@...radead.org>, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org, linux-block@...r.kernel.org
Subject: Re: iio, syfs, devres: devm_kmalloc not aligned to pow2 size argument

On Thu, 14 Nov 2024 20:25:59 +0800, Zijun Hu <zijun_hu@...oud.com> wrote:
> On 2024/11/14 19:29, Matteo Martelli wrote:
> >>>> The address of a chunk allocated with `kmalloc` is aligned to at least
> >>>> ARCH_KMALLOC_MINALIGN bytes. For sizes of power of two bytes, the
> >>>> alignment is also guaranteed to be at least to the respective size.
> >>>>
> >>>> To do so I was thinking to try to move the devres metadata after the
> >>>> data buffer, so that the latter would directly correspond to pointer
> >>>> returned by kmalloc. I then found out that it had been already suggested
> >>>> previously to address a memory optimization [2]. Thus I am reporting the
> >>>> issue before submitting any patch as some discussions might be helpful
> >>>> first.
> >>>>
> >> no, IMO, that is not good idea absolutely.
> > It’s now quite clear to me that the issue is a rare corner case, and the
> > potential impact of making such a change does not justify it. However,
> > for completeness and future reference, are there any additional reasons
> > why this change is a bad idea?
> 
> 1)
> as i ever commented, below existing APIs is very suitable for your
> requirements. right ?
> addr = devm_get_free_pages(dev, GFP_KERNEL|__GFP_ZERO, 0);
> devm_free_pages(dev,addr);

Yes, but I was concerned by the possibility that other users assumed by
mistake that devm_kmalloc() would have provided the same alignment
guarantees as kmalloc(), so at that point a more generic approach could
have been worth a consideration. Given that today the issue seems to be
confined in only one IIO driver it's clearly a corner case and it is
just a matter of fixing that driver by using kmalloc()+devred_add(), or
devm_get_free_pages() as you suggested, instead of using devm_kmalloc().

> 
> 2)
> touching existing API which have been used frequently means high risk?

Indeed. Same answer for 1) applies here.

> 
> 3) if you put the important metadata at the end of the memory block.
>    3.1) it is easy to be destroyed by out of memory access.

This is a good point.

>    3.2) the API will be used to allocate memory with various sizes
>         how to seek the tail metadata ?  is it easy to seek it?

Apparently yes, but likely very hacky by using ksize(). See
data2devres() in [2] for an example.

>    3.3) if you allocate one page, the size to allocate is page size
>         + meta size, it will waste memory align.

I think this is already the case with the current devm_kmalloc().

> 4) below simple alternative is better than your idea. it keep all
> attributes of original kmalloc(). right ?
> 
> static int devres_raw_kmalloc_match(struct device *dev, void *res, void *p)
> {
> 	void **ptr = res;
> 	return *ptr == p;
> }
> 
> static void devres_raw_kmalloc_release(struct device *dev, void *res)
> {
> 	void **ptr = res;
> 	kfree(*ptr);
> }
> 
> void *devm_raw_kmalloc(struct device *dev, size_t size, gfp_t gfp)
> {
> 	void **ptr;
> 	
> 	ptr = devres_alloc(devres_raw_kmalloc_release, sizeof(*ptr), GFP_KERNEL);
> 	f (!ptr)
> 		return NULL;
> 	
> 	*ptr = kmalloc(size, gfp);
> 	if (!*ptr) {
> 		devres_free(ptr);
> 		return NULL;
> 	}
> 	devres_add(dev, ptr);
> 	return *ptr;
> }
> EXPORT(...)
> 
> void *devm_raw_kfree(struct device *dev, void *p)
> {
> 	devres_release(dev, devres_raw_kmalloc_release,
> devres_raw_kmalloc_match, p);
> }
> EXPORT(...)

I also considered an alternative to decouple the two allocations of the
devres metadata and the actual buffer as you suggested here. However, I
would have preferred avoiding an additional API and applying this
approach directly within the original devres_kmalloc() if it turned out
to be necessary. At that point, though, I am not sure which of the two
approaches would have had less impact.

Thanks for sharing this, it could be useful if a similar discussion
arises in future.


>>>> [2]: https://lore.kernel.org/all/20191220140655.GN2827@hirez.programming.kicks-ass.net/

Best regards,
Matteo Martelli