netdev - Re: [PATCH net-next] tls: Fix improper revert in zerocopy_from

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: <20180724201941.GB97592@doronrk-mbp.dhcp.thefacebook.com>
Date:   Tue, 24 Jul 2018 13:19:41 -0700
From:   Doron Roberts-Kedes <doronrk@...com>
To:     Vakul Garg <vakul.garg@....com>
CC:     "David S . Miller" <davem@...emloft.net>,
        Dave Watson <davejwatson@...com>,
        Matt Mullins <mmullins@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] tls: Fix improper revert in zerocopy_from_iter

On Tue, Jul 24, 2018 at 05:13:26AM +0000, Vakul Garg wrote:
> 
> > -----Original Message-----
> > From: Doron Roberts-Kedes [mailto:doronrk@...com]
> > Sent: Tuesday, July 24, 2018 3:50 AM
> > @@ -811,6 +809,7 @@ int tls_sw_recvmsg(struct sock *sk,
> >  			    likely(!(flags & MSG_PEEK)))  {
> >  				struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> >  				int pages = 0;
> > +				int orig_chunk = chunk;
> > 
> >  				zc = true;
> >  				sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> > @@ -820,9 +819,11 @@ int tls_sw_recvmsg(struct sock *sk,
> >  				err = zerocopy_from_iter(sk, &msg-
> > >msg_iter,
> >  							 to_copy, &pages,
> >  							 &chunk, &sgin[1],
> > -							 MAX_SKB_FRAGS,
> > 	false, true);
> > -				if (err < 0)
> > +							 MAX_SKB_FRAGS,
> > 	false);
> > +				if (err < 0) {
> > +					iov_iter_revert(&msg->msg_iter,
> > chunk - orig_chunk);
> >  					goto fallback_to_reg_recv;
> > +				}
> 
> This assumes that msg_iter gets advanced even if zerocopy_from_iter() fails.

Not sure I see what you mean. If msg_iter is not advanced then chunk -
orig_chunk is 0, and the revert is a no-op. 

> It is easier from code readability perspective if functions upon failure do not leave any side effects for the caller to clean-up.
> I suggest that iov_iter_revert() should be called from zerocopy_from_iter() itself if it is going to fail. 

Agreed that zerocopy_from_iter() should call iov_iter_revert(). I didn't
do that because at first glace, the tx path seems to depend on the
iov_iter_revert() being called as a result of either failed
zerocopy_from_iter() or tls_push_record(). However, I think the latter path
cannot actually be taken because tls_push_record appears never to return
a positive value. This means that the code between
the continue and fallback_to_reg_send should probably just be replaced
with simply, goto send_end. 

After that change, its clear that iov_iter_revert() can be safely moved
inside zerocopy_from_iter() in the error case. 

Pending your input, I'll plan on putting up a 2 part patch addressing
each of these.