netdev - Re: ipx: fix locking regression in ipx_sendmsg and ipx

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: <1609909.1jJeqOzgZ8@wuerfel>
Date:	Wed, 19 Nov 2014 09:32:54 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Jiri Bohac <jbohac@...e.cz>
Cc:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg

On Tuesday 18 November 2014 23:10:57 Jiri Bohac wrote:
> On Tue, Nov 18, 2014 at 02:37:26PM +0100, Arnd Bergmann wrote:
> > Does ipxrtr_route_packet() actually sleep while waiting for the network,
> > or is it possible that you only need to change the recvmsg path?
> 
> You're right. 
> In fact, it can sleep in sock_alloc_send_skb(), but my patch does
> not fix this - it releases the lock after that.
> So let's ignore that for now, I'll send a V2 modifying only
> ipx_recvmsg().

Ok.

> > >  	if (sock_flag(sk, SOCK_ZAPPED))
> > > -		goto out;
> > > +		goto out_release;
> > >  
> > > +	release_sock(sk);
> > >  	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
> > >  				flags & MSG_DONTWAIT, &rc);
> > >  	if (!skb) {
> > 
> > Same thing here: I think your patch could be simplified if you just
> > release the socket lock before calling skb_recv_datagram and get
> > it back afterwards, 
> 
> It would simplify the code a little to just get the lock again.
> But do we really want to optimize for source code size at the cost of
> taking locks that are not necessarry?

I'm more interested in the code structure, in particular this bit

@@ -1807,8 +1812,10 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 
        rc = skb_copy_datagram_iovec(skb, sizeof(struct ipxhdr), msg->msg_iov,
                                     copied);
-       if (rc)
-               goto out_free;
+       if (rc) {
+               skb_free_datagram(sk, skb);
+               goto out;
+       }


after your change mixes coding styles: in some failure cases you just goto
a cleanup part, in other cases you do the cleanup before the goto.

If I'm reading the patch correctly, this change has introduced a leak
because you no longer call skb_free_datagram() in the success case.
Changing the locking only around the skb_recv_datagram() call would
have avoided this problem or at least (if I'm reading it wrong and
the patch is indeed correct) have made it easier to review what the
new code flow and what the change is.

> > and it would be much simpler if you could show that the lock is
> > not needed at all.
> 
> At least the ipxitf_insert_socket() inside __ipx_bind() looks
> like it must be protected not to corrupt the intrfc->if_sklist.
> I am not familiar with the code, so there may be other things.

Ok. Better to do just a less invasive change then. Clearly this code
is not getting much testing, so leaving the locking in place (aside
from fixing the bug) seems the better choice.

	Arnd
--
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