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

[zzjas98 at gmail.com]

On 3/20/24 7:27 AM, Andrew Lunn <andrew at 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.

However, we are not sure if i2c_quirk, used in an init_machine hook, has any special assumption, so would appreciate your knowledge to decide whether an NULL check is needed.

Best,
Zijie