Re: [arch/arm/mach-mvebu] Question about kzalloc NULL check in i2c_quirk

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Mar 20, 2024 at 01:48:26PM -0500, zzjas98@gmail.com wrote:
> On 3/20/24 7:27 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Tue, Mar 19, 2024 at 10:03:22PM -0500, Zijie Zhao wrote:
> > > Dear ARM/Marvell maintainers,
> > >
> > > We came across an unusual usage of kzalloc in
> > > arch/arm/mach-mvebu/board-v7.c, function i2c_quirk:
> > >
> > > https://elixir.bootlin.com/linux/v6.8/source/arch/arm/mach-mvebu/board-v7.c#L127
> > > ```
> > > static void __init i2c_quirk(void)
> > > {
> > > 	struct device_node *np;
> > > 	u32 dev, rev;
> > >
> > > 	/*
> > > 	 * Only revisons more recent than A0 support the offload
> > > 	 * mechanism. We can exit only if we are sure that we can
> > > 	 * get the SoC revision and it is more recent than A0.
> > > 	 */
> > > 	if (mvebu_get_soc_id(&dev, &rev) == 0 && rev > MV78XX0_A0_REV)
> > > 		return;
> > >
> > > 	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c") {
> > > 		struct property *new_compat;
> > >
> > > 		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
> > >
> > > 		new_compat->name = kstrdup("compatible", GFP_KERNEL);
> > > 		new_compat->length = sizeof("marvell,mv78230-a0-i2c");
> > > 		new_compat->value = kstrdup("marvell,mv78230-a0-i2c",
> > > 						GFP_KERNEL);
> > >
> > > 		of_update_property(np, new_compat);
> > > 	}
> > > }
> > > ```
> > >
> > > Should the new_compat be checked against NULL in case kzalloc fails, to
> > > avoid NULL dereference later in the code?
> > >
> > > Please kindly let us know if we missed any key information and this is
> > > actually intended. We appreciate your information and time! Thanks!
> > 
> > What context is this code run in? What would need to happen for the
> > allocation to fail? And what happens next if it does fail, both in
> > this function and the system in general?
> > 
> > 	   Andrew
> > 
> 
> Hi Andrew,
> 
> Thanks for checking!
> 
> We encountered this kzalloc while doing a static analysis for the kernel code.
> 
> kzalloc would return NULL in case of out-of-memory and would make the next field access new_compat->name segfault.

If we are out of memory at this point in the kernel boot, then what are
the chances of the kernel getting to the point where it can run
userspace?

I think that is the essence of Andrew's argument. A failure to allocate
memory here means there is a serious problem and the system won't boot
_even_ if we add something like:

	if (!new_compat)
		continue;

to that loop.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel