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

[Zijie Zhao]

On 3/20/24 2:46 PM, Russell King (Oracle) wrote:
> On Wed, Mar 20, 2024 at 01:48:26PM -0500, zzjas98 at gmail.com wrote:
>> 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.
> 
> 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.
> 


Hi Andrew and Russell,

Thanks for pointing out the consequence! It makes sense and we will 
try to make our analyzer realize this as well. Thanks again for your time!

Best,
Zijie