netdev - Re: [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet

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: <52BAB246.7060709@redhat.com>
Date:	Wed, 25 Dec 2013 11:24:06 +0100
From:	Daniel Borkmann <dborkman@...hat.com>
To:	Wang Weidong <wangweidong1@...wei.com>
CC:	Neil Horman <nhorman@...driver.com>,
	Vlad Yasevich <vyasevich@...il.com>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	"linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>
Subject: Re: [PATCH net-next] sctp: check dst 'NULL' before use it in, sctp_packet_transmit

On 12/25/2013 08:47 AM, Wang Weidong wrote:
> skb_dst_set will use dst, if dst is NULL although is not a problem,
> then goto the no_route and free nskb, so do the skb_dst_set is pointless.
> so check dst before use it. Remove the unnecessary initialization as well.

Please also cc linux-sctp as you did before!

Just went through the code, only reading from your subject title it first
sounded like a NULL pointer dereference, but in fact the code is fine and
nothing is wrong with it. I'd suggest you should make the subject sound
more "harmless" to not confuse people, imho, since all you do here is some
cleanup and rearrangement. "Use" sounds to me as dereferencing dst that is
NULL.

> Signed-off-by: Wang Weidong <wangweidong1@...wei.com>
> ---
>   net/sctp/output.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 3be70a4..9b76d62 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -387,7 +387,7 @@ int sctp_packet_transmit(struct sctp_packet *packet)
>   	int err = 0;
>   	int padding;		/* How much padding do we need?  */
>   	__u8 has_data = 0;
> -	struct dst_entry *dst = tp->dst;
> +	struct dst_entry *dst;
>   	unsigned char *auth = NULL;	/* pointer to auth in skb data */
>
>   	pr_debug("%s: packet:%p\n", __func__, packet);
> @@ -420,9 +420,9 @@ int sctp_packet_transmit(struct sctp_packet *packet)
>   		}
>   	}
>   	dst = dst_clone(tp->dst);
> -	skb_dst_set(nskb, dst);
>   	if (!dst)
>   		goto no_route;

Nit: you should set a newline here.

> +	skb_dst_set(nskb, dst);
>
>   	/* Build the SCTP header.  */
>   	sh = (struct sctphdr *)skb_push(nskb, sizeof(struct sctphdr));
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html