netdev - Re: [PATCH v2] ice: Fix improper handling of refcount in ice_sriov_set_msix_vec

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240906083038.GC2097826@kernel.org>
Date: Fri, 6 Sep 2024 09:30:38 +0100
From: Simon Horman <horms@...nel.org>
To: Gui-Dong Han <hanguidong02@...look.com>
Cc: anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, intel-wired-lan@...ts.osuosl.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	baijiaju1990@...il.com, stable@...r.kernel.org
Subject: Re: [PATCH v2] ice: Fix improper handling of refcount in
 ice_sriov_set_msix_vec_count()

On Tue, Sep 03, 2024 at 11:59:43AM +0000, Gui-Dong Han wrote:
> This patch addresses an issue with improper reference count handling in the
> ice_sriov_set_msix_vec_count() function.
> 
> First, the function calls ice_get_vf_by_id(), which increments the
> reference count of the vf pointer. If the subsequent call to
> ice_get_vf_vsi() fails, the function currently returns an error without
> decrementing the reference count of the vf pointer, leading to a reference
> count leak. The correct behavior, as implemented in this patch, is to
> decrement the reference count using ice_put_vf(vf) before returning an
> error when vsi is NULL.
> 
> Second, the function calls ice_sriov_get_irqs(), which sets
> vf->first_vector_idx. If this call returns a negative value, indicating an
> error, the function returns an error without decrementing the reference
> count of the vf pointer, resulting in another reference count leak. The
> patch addresses this by adding a call to ice_put_vf(vf) before returning
> an error when vf->first_vector_idx < 0. 
> 
> This bug was identified by an experimental static analysis tool developed
> by our team. The tool specializes in analyzing reference count operations
> and identifying potential mismanagement of reference counts. In this case,
> the tool flagged the missing decrement operation as a potential issue,
> leading to this patch.
> 
> Fixes: 4035c72dc1ba ("ice: reconfig host after changing MSI-X on VF")
> Fixes: 4d38cb44bd32 ("ice: manage VFs MSI-X using resource tracking")
> Cc: stable@...r.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@...look.com>
> ---
> v2:
> * In this patch v2, an additional resource leak was addressed when
> vf->first_vector_idx < 0. The issue is now fixed by adding ice_put_vf(vf)
> before returning an error.
>   Thanks to Simon Horman for identifying this additional leak scenario.

Thanks for the update,

I agree with the analysis and that the two instances of
this problem were introduced by each of the cited commits.

Reviewed-by: Simon Horman <horms@...nel.org>