netdev - Re: Undefined behaviour of connect(fd, NULL, 0);

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: <20100401143805.1f83a161@notabene.brown>
Date:	Thu, 1 Apr 2010 14:38:05 +1100
From:	Neil Brown <neilb@...e.de>
To:	Changli Gao <xiaosuo@...il.com>
Cc:	David Miller <davem@...emloft.net>, shemminger@...tta.com,
	netdev@...r.kernel.org
Subject: Re: Undefined behaviour of connect(fd, NULL, 0);

On Thu, 1 Apr 2010 11:00:23 +0800
Changli Gao <xiaosuo@...il.com> wrote:

> I think the following patch is what Neil wants. The old code implies that
> connect(fd, NULL, 0) is used to check the socket connecting status, but
> Stephen's patch breaks it. The old code is wrong when it checks the address's
> faimly but not check the sizeof of the address to determine the family member
> is valid or not before.
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index be1a6ac..3ff51f0 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -576,7 +576,8 @@ int inet_stream_connect(struct socket *sock,
> struct sockaddr *uaddr,
> 
>  	lock_sock(sk);
> 
> -	if (uaddr->sa_family == AF_UNSPEC) {
> +	if (addr_len >= sizeof(uaddr->sa_family) &&
> +	    uaddr->sa_family == AF_UNSPEC) {
>  		err = sk->sk_prot->disconnect(sk, flags);
>  		sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
>  		goto out;

I'm not sure I'd say that I "want" any particular patch.
I just want to know what "connect(fd, NULL, 0)" is supposed to do, and to
have the kernel be consistent in its behaviour.  I'm not really fussed what
the behaviour is.

I suspect the customer wants that patch you have supplied as it would mean
they don't need to change their code.  But I only want it if it is "right".

The patch you have provided does what I had assumed Stephen's patch did
before I actually read it properly.

My feeling is that this patch might be more useful than Stephen's as having
connect(fd, NULL, 0) do what the customer expects seems useful, where as
having it do the same as setting AF_UNSPEC doesn't add anything.

I've googled around a bit but cannot find any evidence of anyone passing NULL
to connect like this, and what documentation I can find doesn't really
address the issue at all.

Thanks,
NeilBrown
--
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