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

Message-ID: <20191104113407.7da3ed44@cakuba.netronome.com>
Date:   Mon, 4 Nov 2019 11:34:07 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     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

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.

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

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?