Re: [PATCH 2/2] devres: Provide kmalloc_obj*-style API for devm_kmalloc

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5742 bytes --]

[CC += Linus]

Hi Laurent, Linus,

> Date: 2026-11-12 02:40:39+0200
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Hi Kees,
> 
> On Mon, Sep 14, 2026 at 03:32:17PM -0700, Kees Cook wrote:
> > Mirroring the type-based allocation now in use for the kmalloc family of
> > APIs, provide the same for the devm_kmalloc family of APIs. This will
> > allow for future plumbing of type information (e.g. alignment) into the
> > underlying allocations. This immediately gains type check on return
> > values (instead of the prior "void *" return type).
> > 
> > Signed-off-by: Kees Cook <kees@kernel.org>
> > ---
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> > Cc: <driver-core@lists.linux.dev>
> > Cc: <linux-hardening@vger.kernel.org>
> > ---
> >  include/linux/device/devres.h | 102 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> > 
[...]
> > +#define devm_kmalloc_flex(DEV, VAR_OR_TYPE, FAM, COUNT, ...) \
> > +	__devm_alloc_flex(devm_kmalloc, DEV, default_gfp(__VA_ARGS__), \
> > +			  typeof(VAR_OR_TYPE), FAM, COUNT)
> > +
> > +/* All devm_kzalloc aliases for devm_kmalloc_(obj|objs|flex). */
> > +#define devm_kzalloc_obj(DEV, P, ...) \
> > +	__devm_alloc_objs(devm_kzalloc, DEV, default_gfp(__VA_ARGS__), typeof(P), 1)
> > +#define devm_kzalloc_objs(DEV, P, COUNT, ...) \
> > +	__devm_alloc_objs(devm_kzalloc, DEV, default_gfp(__VA_ARGS__), \
> > +			  typeof(P), COUNT)
> 
> I was recently made aware by Geert of an unfortunate side effect of the
> default GPF_KERNEL argument. I've fixed a memory leak in the imx8-isi
> driver by using devm_kzalloc_objs() as follows:
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> index 0ae5f35e017c..9373a5256224 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> @@ -488,7 +488,8 @@ static int mxc_isi_probe(struct platform_device *pdev)
> 
>  	isi->pdata = of_device_get_match_data(dev);
> 
> -	isi->pipes = kzalloc_objs(isi->pipes[0], isi->pdata->num_channels);
> +	isi->pipes = devm_kzalloc_objs(dev, isi->pipes[0],
> +				       isi->pdata->num_channels);
>  	if (!isi->pipes)
>  		return -ENOMEM;
> 
> So far, so good. However, the following code also compiles:
> 
> 	isi->pipes = devm_kzalloc_obj(dev, isi->pipes[0],
> 				      isi->pdata->num_channels);
> 
> (note the *obj* instead of *objs*) because the third argument is
> interpreted as gfp flags. It's clearly a bug, but one that the compiler
> doesn't catch. The typo is easy to make.
> 
> Sparse catches the issue:
> 
> drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c:491:22: warning: incorrect type in argument 3 (different base types)
> drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c:491:22:    expected restricted gfp_t [usertype] gfp
> drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c:491:22:    got unsigned int const num_channels
> 
> I'm however not sure if build bots report sparse warnings or only
> errors.
> 
> Is this an issue you're aware of, and do you think it should be
> addressed ?

Indeed, this is a known problem of these macros.  I reported it in March:
<https://lore.kernel.org/lkml/abhGS0n_RsUG97Ni@devuan/>

The link contains a detailed report of the issue, and why it happens.

There is not a single cause of this bug, but rather a combination of
several of them.

I suggest removing kmalloc_obj() entirely, and using kmalloc_objs()
always, with a second argument of 1 when we want a single object.  This
would remove the risk of that typo.  That is, instead of:

    int *p = kmalloc_objs(int, 42);
    int *q = kmalloc_obj(int);

we should do

    int *p = kmalloc_objs(int, 42);
    int *q = kmalloc_objs(int, 1);

I also suggest having default arguments without variadic macros.  The
way to do that is using empty arguments.  Linus complained that it hurts
his eyes, but it really makes these macros much safer.

That is, instead of

	int *p = kmalloc_objs(int, 42);

we should do

	int *p = kmalloc_objs(int, 42,);

By having an explicitly empty third parameter, we'd be asking explicitly
for the default value, which the compiler can verify, compared to having
to guess whether we asked for it or we just forgot about its existence.

Any of these two changes would already reduce the chances of this bug
significantly, but if we apply both, it's almost impossible to have it.

Another issue with that macro is accepting an expression instead of a
type name.  Consider the following mistake:

    p = kmalloc_objs(42, *p);

The compiler can't catch that we swapped the order of arguments
accidentally, and will allocate a wrong number of objects (whatever
happens to be in *p, if already initialized, else we might be lucky and
get an uninitialized diagnostic).  If we require that the first
parameter is a type name, it will preclude that problem.

Please let me know if any of these changes is wanted, and I can prepare
some patches.


Have a lovely day!
Alex

> 
> > +#define devm_kzalloc_flex(DEV, P, FAM, COUNT, ...) \
> > +	__devm_alloc_flex(devm_kzalloc, DEV, default_gfp(__VA_ARGS__), \
> > +			  typeof(P), FAM, COUNT)
> > +
> >  static inline __realloc_size(3, 4) void * __must_check
> >  devm_krealloc_array(struct device *dev, void *p, size_t new_n, size_t new_size, gfp_t flags)
> >  {
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]