[PATCH net] ice: Adjust memory overrun in ice_sched_add_root_node() and ice_sched_add_node()

[PATCH net] ice: Adjust memory overrun in ice_sched_add_root_node() and ice_sched_add_node()

[Aleksandr Mishin]

In ice_sched_add_root_node() and ice_sched_add_node() there are calls to
devm_kcalloc() in order to allocate memory for array of pointers to
'ice_sched_node' structure. But in this calls there are 'sizeof(*root)'
instead of 'sizeof(root)' and 'sizeof(*node)' instead of 'sizeof(node)'.
So memory is allocated for structures instead pointers. This lead to
significant memory overrun.
Looks like it was done for "coverity[suspicious_sizeof] workaround".

Adjust memory overrun by correcting devm_kcalloc() parameters.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: dc49c7723676 ("ice: Get MAC/PHY/link info and scheduler topology")
Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
---
 drivers/net/ethernet/intel/ice/ice_sched.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index ecf8f5d60292..d8b6054f3436 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -28,9 +28,8 @@ ice_sched_add_root_node(struct ice_port_info *pi,
 	if (!root)
 		return -ENOMEM;
 
-	/* coverity[suspicious_sizeof] */
 	root->children = devm_kcalloc(ice_hw_to_dev(hw), hw->max_children[0],
-				      sizeof(*root), GFP_KERNEL);
+				      sizeof(root), GFP_KERNEL);
 	if (!root->children) {
 		devm_kfree(ice_hw_to_dev(hw), root);
 		return -ENOMEM;
@@ -186,10 +185,9 @@ ice_sched_add_node(struct ice_port_info *pi, u8 layer,
 	if (!node)
 		return -ENOMEM;
 	if (hw->max_children[layer]) {
-		/* coverity[suspicious_sizeof] */
 		node->children = devm_kcalloc(ice_hw_to_dev(hw),
 					      hw->max_children[layer],
-					      sizeof(*node), GFP_KERNEL);
+					      sizeof(node), GFP_KERNEL);
 		if (!node->children) {
 			devm_kfree(ice_hw_to_dev(hw), node);
 			return -ENOMEM;
-- 
2.30.2

Re: [PATCH net] ice: Adjust memory overrun in ice_sched_add_root_node() and ice_sched_add_node()

[Simon Horman]

On Fri, Jul 05, 2024 at 07:36:20PM +0300, Aleksandr Mishin wrote:
> In ice_sched_add_root_node() and ice_sched_add_node() there are calls to
> devm_kcalloc() in order to allocate memory for array of pointers to
> 'ice_sched_node' structure. But in this calls there are 'sizeof(*root)'
> instead of 'sizeof(root)' and 'sizeof(*node)' instead of 'sizeof(node)'.
> So memory is allocated for structures instead pointers. This lead to
> significant memory overrun.
> Looks like it was done for "coverity[suspicious_sizeof] workaround".

Hi Aleksandr,

While I agree that your patch is correct, I doesn't look to me like it was
done for "coverity[suspicious_sizeof] workaround", as that annotation was
added after the cited patch where the problem appears to have been
introduced.

> 
> Adjust memory overrun by correcting devm_kcalloc() parameters.

I also think it is misleading to describe this as an overrun.  In my
opinion, an overrun refers to writing over the end of a buffer, or similar
conditions where values are written to memory or read from that is not
intended for that purpose.

But that is not the case here.  Instead it is an over allocation of memory.
Which is, in a sense, the opposite of an overrun. I suggest updating the
description of the problem.

> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: dc49c7723676 ("ice: Get MAC/PHY/link info and scheduler topology")

I do agree there is a problem. But I'm not convinced this is fixing a bug -
is the overallocation of memory manifesting, in a real problem, other than
perhaps contributing to memory pressure (I assume in not a very significant
way).

My feeling is that it would be better to target this change to net-next and
drop the Fixes tag.

> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>

...