linux-kernel - Re: [PATCH][next] xfrm: Add error handling when nla_put

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: <2092f824-b60e-4d78-991b-61b35a312597@intel.com>
Date: Wed, 13 Nov 2024 15:12:02 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Everest K.C. <everestkc@...restkc.com.np>, <horms@...nel.org>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>
CC: <netdev@...r.kernel.org>, <kernel-janitors@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <steffen.klassert@...unet.com>,
	<herbert@...dor.apana.org.au>
Subject: Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns
 an error

On 11/13/24 00:36, Everest K.C. wrote:
> Error handling is missing when call to nla_put_u32() fails.
> Handle the error when the call to nla_put_u32() returns an error.
> 
> The error was reported by Coverity Scan.
> Report:
> CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
> returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
> to err here, but that stored value is overwritten before it can be used
> 
> Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> Signed-off-by: Everest K.C. <everestkc@...restkc.com.np>
> ---
>   net/xfrm/xfrm_user.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index f0ee0c7a59dd..a784598cc7cf 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2607,9 +2607,12 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
>   	err = xfrm_if_id_put(skb, x->if_id);
>   	if (err)
>   		goto out_cancel;
> -	if (x->pcpu_num != UINT_MAX)
> +	if (x->pcpu_num != UINT_MAX) {
>   		err = nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num);
> -
> +		if (err)
> +			goto out_cancel;
> +	}
> +
>   	if (x->dir) {
>   		err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
>   		if (err)

this is a fix indeed,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
--
I find nla_put*() familiy error handling very ugly for the calling code,
especially given that some of the calls are conditional

I would like to refactor it some day, to give the caller possibility to
just put all the needed fields and check the error once at the end.
Nesting complicates things a bit, but perhaps it could be also covered
in such way (didn't checked yet).