Re: [PATCH 1/2] lib/string_helpers: drop redundant allocation in kasprintf_strarray

[Kees Cook <kees@kernel.org>]

On Thu, Apr 30, 2026 at 04:39:47PM +0200, Thorsten Blum wrote:
> Hi Kees and Andrew,
> 
> On Thu, Apr 16, 2026 at 10:48:51AM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 15, 2026 at 05:30:50PM +0200, Thorsten Blum wrote:
> > > On Wed, Apr 15, 2026 at 05:42:41PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Apr 15, 2026 at 02:25:43PM +0200, Thorsten Blum wrote:
> > > > > kasprintf_strarray() returns an array of N strings and kfree_strarray()
> > > > > also frees N entries.  However, kasprintf_strarray() currently allocates
> > > > > N+1 char pointers.  Allocate exactly N pointers instead of N+1.
> > > > > 
> > > > > Also update the kernel-doc for @n.
> > > > 
> > > > Have you checked all current users that they do not rely on the NULL terminated
> > > > array?
> > > 
> > > Yes, I've checked all call sites, and none of them rely on the NULL
> > > terminator. Specifically, I checked:
> > > 
> > >   drivers/gpio/gpio-mockup.c
> > > 
> > > which uses PROPERTY_ENTRY_STRING_ARRAY_LEN(), and
> > > 
> > >   drivers/pinctrl/bcm/pinctrl-bcm4908.c
> > >   drivers/pinctrl/intel/pinctrl-intel-platform.c
> > >   drivers/pinctrl/meson/pinctrl-amlogic-a4.c
> > >   drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> > >   drivers/pinctrl/pinctrl-at91.c
> > >   drivers/pinctrl/pinctrl-rockchip.c
> > >   drivers/pinctrl/pinctrl-st.c
> > > 
> > > all of which use the size N to iterate over the returned array.
> > 
> > Thanks for confirming.
> > 
> > > Also, kfree_strarray() explicitly takes the number of entries N,
> > > indicating that callers are expected to keep track of it.
> > 
> > Still we might have an API that requires a NULL terminated arrays (when it
> > doesn't take size), which a caller wants to use.

What I find problematic here is that we allocate N+1 and free N. And
this is repeated in wrappers too, like devm_kasprintf_strarray(), which
explicitly tracks N for the later devm free. How has kmemleak missed
this?

> > > > Note, that was done on purpose that once allocated it can allow user
> > > > to drop the track of the number of strings and rely on NULL terminator.
> > > > I.o.w.  the number of strings may be just a local variable somewhere
> > > > where kasprintf_strarray() is called.
> > > > 
> > > > I tend to NAK this change, rather you can update kernel-doc to explain
> > > > why it's done this way (see above).
> > 
> > Given pros and cons, and what David said I'm still not sure that this is
> > going to be a beneficial patch. I leave it Kees and Andrew to decide.
> 
> What's your take on this and the __counted_by_ptr() annotation from
> patch 2/2?

I think both patches look good.

-- 
Kees Cook