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

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

[Zijie Zhao]

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!

Best,
Zijie

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

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

[Andrew Lunn]

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

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

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

[zzjas98]

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.

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

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

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

[Andrew Lunn]

> > 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.

Please configure your email client to wrap lines at around 70
characters.

O.K, let me help you answer your questions.

When is init_machine called?

What would need to happen for the allocation to fail?

Say it does fall, and you avoid a NULL pointer dereference here. What
happens next to the system in general.

These are not hard questions to answer, you just need to think about
them a little.

     Andrew

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

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

[Russell King (Oracle)]

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

Re: [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@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.
> 


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

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

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

[Andrew Lunn]

> > 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!

If you can make the analyzer realise this, that would be great.

If not, feel free to repost this patch, but please rework the commit
message. Point out that checking for NULL is pointless here, the
system is doomed. However, the static analysers cannot determine
that. And it makes sense to silence this pointless warning to stop it
hiding other real warning which could be real issues. "Cannot see the
forest because of the trees".

Something else to consider. The system is doomed. So maybe call
panic()?

	Andrew

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

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

[Zijie Zhao]

On 3/20/24 8:10 PM, Andrew Lunn wrote:
>>> 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!
> 
> If you can make the analyzer realise this, that would be great.
> 
> If not, feel free to repost this patch, but please rework the commit
> message. Point out that checking for NULL is pointless here, the
> system is doomed. However, the static analysers cannot determine
> that. And it makes sense to silence this pointless warning to stop it
> hiding other real warning which could be real issues. "Cannot see the
> forest because of the trees".
> 
> Something else to consider. The system is doomed. So maybe call
> panic()?
> 
> 	Andrew

Hi Andrew,

Thanks for considering help our analyzer. We examined more similar 
cases and found there are a few other locations where OOM equals dead 
system, so it would probably be better to improve the analyzer instead 
of adding more checks.

Thanks again for the information and your time!

Best,
Zijie

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