netdev - Re: [PATCH net-next 1/3] net: mctp: handle skb cleanup on sock

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: <f3dba541-8880-4a03-b0c9-e7b9b552b8f3@redhat.com>
Date: Tue, 17 Dec 2024 09:15:07 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jeremy Kerr <jk@...econstruct.com.au>,
 Matt Johnston <matt@...econstruct.com.au>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net: mctp: handle skb cleanup on sock_queue
 failures

On 12/11/24 06:56, Jeremy Kerr wrote:
> Currently, we don't use the return value from sock_queue_rcv_skb, which
> means we may leak skbs if a message is not successfully queued to a
> socket.
> 
> Instead, ensure that we're freeing the skb where the sock hasn't
> otherwise taken ownership of the skb by adding checks on the
> sock_queue_rcv_skb() to invoke a kfree on failure.
> 
> In doing so, rather than using the 'rc' value to trigger the
> kfree_skb(), use the skb pointer itself, which is more explicit.
> 
> Also, add a kunit test for the sock delivery failure cases.
> 
> Signed-off-by: Jeremy Kerr <jk@...econstruct.com.au>

Why are you targeting net-next for this patch? it looks like a clean fix
for net, and follow-up patches don't depend on it.

> ---
>  net/mctp/route.c           | 38 +++++++++++++-------
>  net/mctp/test/route-test.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+), 12 deletions(-)
> 
> diff --git a/net/mctp/route.c b/net/mctp/route.c
> index 597e9cf5aa64445474287a3fee02ba760db15796..49676ce627e30ee34924d64fe26ef1e0303518d9 100644
> --- a/net/mctp/route.c
> +++ b/net/mctp/route.c
> @@ -374,8 +374,13 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
>  	msk = NULL;
>  	rc = -EINVAL;
>  
> -	/* we may be receiving a locally-routed packet; drop source sk
> -	 * accounting
> +	/* We may be receiving a locally-routed packet; drop source sk
> +	 * accounting.
> +	 *
> +	 * From here, we will either queue the skb - either to a frag_queue, or
> +	 * to a receiving socket. When that succeeds, we clear the skb pointer;
> +	 * a non-NULL skb on exit will be otherwise unowned, and hence
> +	 * kfree_skb()-ed.
>  	 */
>  	skb_orphan(skb);
>  
> @@ -434,7 +439,9 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
>  		 * pending key.
>  		 */
>  		if (flags & MCTP_HDR_FLAG_EOM) {
> -			sock_queue_rcv_skb(&msk->sk, skb);
> +			rc = sock_queue_rcv_skb(&msk->sk, skb);
> +			if (!rc)
> +				skb = NULL;
>  			if (key) {
>  				/* we've hit a pending reassembly; not much we
>  				 * can do but drop it
> @@ -443,7 +450,6 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
>  						   MCTP_TRACE_KEY_REPLIED);
>  				key = NULL;
>  			}
> -			rc = 0;
>  			goto out_unlock;
>  		}
>  
> @@ -470,8 +476,10 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
>  			 * this function.
>  			 */
>  			rc = mctp_key_add(key, msk);
> -			if (!rc)
> +			if (!rc) {
>  				trace_mctp_key_acquire(key);
> +				skb = NULL;
> +			}
>  
>  			/* we don't need to release key->lock on exit, so
>  			 * clean up here and suppress the unlock via
> @@ -489,6 +497,8 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
>  				key = NULL;
>  			} else {
>  				rc = mctp_frag_queue(key, skb);
> +				if (!rc)
> +					skb = NULL;
>  			}
>  		}
>  
> @@ -498,17 +508,22 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
>  		 */
>  
>  		/* we need to be continuing an existing reassembly... */
> -		if (!key->reasm_head)
> -			rc = -EINVAL;
> -		else
> +		if (key->reasm_head)
>  			rc = mctp_frag_queue(key, skb);
> +		else
> +			rc = -EINVAL;

This chunk just re-order existing statement, it looks unneeded and I
would remove it from 'net' fix.

> +
> +		/* if we've queued, the queue owns the skb now */
> +		if (!rc)
> +			skb = NULL;

Possibly:
		if (rc)
			goto out_unlock;

		skb = NULL;
		if (flags & MCTP_HDR_FLAG_EOM) {

is simpler/clearer

Thanks,

Paolo