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] [day] [month] [year] [list]

Date:   Wed, 25 Jul 2018 06:10:07 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Doron Roberts-Kedes <doronrk@...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



> -----Original Message-----
> From: Doron Roberts-Kedes [mailto:doronrk@...com]
> Sent: Wednesday, July 25, 2018 1:50 AM
> 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
> 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.
> 
As I said below, my comment was to improve code readability.
It takes a while to grasp that calling iov_iter_revert would result in no-op
if zerocopy_from_iter() fails.

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

Please submit.