netdev - Re: [PATCH net] ice: Adjust memory overrun in ice_sched_add_root_node() and ice_sched_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Sat, 6 Jul 2024 10:52:58 +0100
From: Simon Horman <horms@...nel.org>
To: Aleksandr Mishin <amishin@...rgos.ru>
Cc: Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH net] ice: Adjust memory overrun in
 ice_sched_add_root_node() and ice_sched_add_node()

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@...rgos.ru>

...