netdev - Re: [PATCH -next] sctp: fix error return code in __sctp

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: <20130403145909.GB32276@hmsreliant.think-freely.org>
Date:	Wed, 3 Apr 2013 10:59:09 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Vlad Yasevich <vyasevich@...il.com>
Cc:	Wei Yongjun <weiyj.lk@...il.com>, davem@...emloft.net,
	yongjun_wei@...ndmicro.com.cn, linux-sctp@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH -next] sctp: fix error return code in __sctp_connect()

On Wed, Apr 03, 2013 at 10:52:16AM -0400, Vlad Yasevich wrote:
> On 04/03/2013 09:51 AM, Neil Horman wrote:
> >On Wed, Apr 03, 2013 at 09:02:28PM +0800, Wei Yongjun wrote:
> >>From: Wei Yongjun <yongjun_wei@...ndmicro.com.cn>
> >>
> >>Fix to return a negative error code from the error handling
> >>case instead of 0, as returned elsewhere in this function.
> >>
> >>Signed-off-by: Wei Yongjun <yongjun_wei@...ndmicro.com.cn>
> >>---
> >>  net/sctp/socket.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> >>index dd21ae3..f631c5f 100644
> >>--- a/net/sctp/socket.c
> >>+++ b/net/sctp/socket.c
> >>@@ -1119,9 +1119,10 @@ static int __sctp_connect(struct sock* sk,
> >>  		/* Make sure the destination port is correctly set
> >>  		 * in all addresses.
> >>  		 */
> >>-		if (asoc && asoc->peer.port && asoc->peer.port != port)
> >>+		if (asoc && asoc->peer.port && asoc->peer.port != port) {
> >>+			err = -EINVAL;
> >>  			goto out_free;
> >>-
> >>+		}
> >>
> >>  		/* Check if there already is a matching association on the
> >>  		 * endpoint (other than the one created here).
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> >>the body of a message to majordomo@...r.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >Actually, I think you can remove that entire if statement (as well as some
> >checks further down).  Looking at the net-next trees __sctp_connect, it appears
> >that asoc is set to NULL at the top of the function, and not assigned to
> >anything else until the call to sctp_association_new much farther down (line
> >1201).  That means the above if statement, as well as this:
> >  if (asoc2 && asoc2 != asoc) {
> >and this:
> >if (!asoc) {
> >will always be false, false, and true, respectively.
> 
> No, I don't think you can.  Consider a case of sctp_connectx() where
> each address specified in connectx has a different destination port.
> 
> First time through the loop, we'll create the association and set
> the peer.port.  The second time through the loop, we'll compare the
> that port to the port specified in the second address.  If the ports
> do not match, we need to stop.
> 
> -vlad
Ah, you're right, I missed the while loop, apologies

Acked-by: Neil Horman <nhorman@...driver.com>

> >
> >Regards
> >Neil
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
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