netdev - Re: [PATCH net] net/tls: fix sk_msg trim on fallback to copy mode

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]

Message-ID: <5dc0ad165502b_42ae2af05d4685c0e9@john-XPS-13-9370.notmuch>
Date:   Mon, 04 Nov 2019 14:58:30 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        oss-drivers@...ronome.com, borisp@...lanox.com,
        aviadye@...lanox.com, daniel@...earbox.net,
        syzbot+f8495bff23a879a6d0bd@...kaller.appspotmail.com,
        syzbot+6f50c99e8f6194bf363f@...kaller.appspotmail.com,
        Eric Biggers <ebiggers@...nel.org>,
        herbert@...dor.apana.org.au, glider@...gle.com,
        linux-crypto@...r.kernel.org
Subject: Re: [PATCH net] net/tls: fix sk_msg trim on fallback to copy mode

Jakub Kicinski wrote:
> On Mon, 04 Nov 2019 10:56:52 -0800, John Fastabend wrote:
> > Jakub Kicinski wrote:
> > > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> > > index cf390e0aa73d..f87fde3a846c 100644
> > > --- a/net/core/skmsg.c
> > > +++ b/net/core/skmsg.c
> > > @@ -270,18 +270,28 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
> > >  
> > >  	msg->sg.data[i].length -= trim;
> > >  	sk_mem_uncharge(sk, trim);
> > > +	/* Adjust copybreak if it falls into the trimmed part of last buf */
> > > +	if (msg->sg.curr == i && msg->sg.copybreak > msg->sg.data[i].length)
> > > +		msg->sg.copybreak = msg->sg.data[i].length;
> > >  out:
> > > -	/* If we trim data before curr pointer update copybreak and current
> > > -	 * so that any future copy operations start at new copy location.
> > > +	sk_msg_iter_var_next(i);
> > > +	msg->sg.end = i;
> > > +
> > > +	/* If we trim data a full sg elem before curr pointer update
> > > +	 * copybreak and current so that any future copy operations
> > > +	 * start at new copy location.
> > >  	 * However trimed data that has not yet been used in a copy op
> > >  	 * does not require an update.
> > >  	 */
> > > -	if (msg->sg.curr >= i) {
> > > +	if (!msg->sg.size) {
> > > +		msg->sg.curr = msg->sg.start;
> > > +		msg->sg.copybreak = 0;
> > > +	} else if (sk_msg_iter_dist(msg->sg.start, msg->sg.curr) >
> > > +		   sk_msg_iter_dist(msg->sg.end, msg->sg.curr)) {  
> > 
> > I'm not seeing how this can work. Taking simple case with start < end
> > so normal geometry without wrapping. Let,
> > 
> >  start = 1
> >  curr  = 3
> >  end   = 4
> > 
> > We could trim an index to get,
> > 
> >  start = 1
> >   curr = 3
> >      i = 3
> >    end = 4
> 
> IOW like this?
> 
> 	test_one(/* start */ 1, /* curr */ 3, /* copybreak */ 150,
> 		 /* trim */ 500,
> 		 /* curr */ 3, /* copybreak */ 100, /* end */ 4,
> 		 /* data */ 200, 200, 200);
> 
> test #13  start:1 curr:3 end:4 cb:150 size: 600      0 200 200 200   0	OKAY
> 
> > Then after out: label this would push end up one,
> > 
> >  start = 1
> >   curr = 3
> >      i = 3
> >    end = 4
> 
> I moved the assignment to end before the curr adjustment, so 'i' is
> equivalent to 'end' at this point.

right.

> 
> > But dist(start,curr) = 2 and dist(end, curr) = 1 and we would set curr
> > to '3' but clear the copybreak?
> 
> I don't think we'd fall into this condition ever, unless we moved end.
> And in your example AFAIU we don't move end.
> 
> > I think a better comparison would be,
> > 
> >   if (sk_msg_iter_dist(msg->sg.start, i) <
> >       sk_msg_iter_dist(msg->sg.start, msg->sg.curr)
> > 
> > To check if 'i' walked past curr so we can reset curr/copybreak?
> 
> Ack, this does read better!

Great.

> 
> Should we use <= here? If we dropped a full segment, should curr point
> at the end of the last remaining segment or should it point at 0 in end?

Right it should be <=.

Full segment? If a segment is trimmed exactly then curr can point to the
previous segment with 'copybreak = sge->length' so next copy will see a
full buffer and advance curr. Or can leave curr on the trim'ed segment
with copybreak set to 0.

Both should be OK as long as copybreak is correct.  Reviewing code now
to be sure we didn't take any shortcuts but that should be true else we
may have other bugs when working with BPF.