netdev - Re: [oss-drivers] Re: [PATCH net] net/tls: fix sk

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: <20191101102238.7f56cb84@cakuba.netronome.com>
Date:   Fri, 1 Nov 2019 10:22:38 -0700
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: [oss-drivers] Re: [PATCH net] net/tls: fix sk_msg trim on
 fallback to copy mode

On Fri, 01 Nov 2019 08:01:00 -0700, John Fastabend wrote:
> Jakub Kicinski wrote:
> > I will look in depth tomorrow as well, the full/empty cases are a
> > little tricky to fold into general logic.
> > 
> > I came up with this before I got distracted Halloweening :)  
> 
> Same here. Looking at the two cases from above.
> 
>    if (msg->sg.start < msg->sg.end &&
>        i < msg->sg.curr) {  // i <= msg->sg.curr
>       msg->sg.curr = i;
>       msg->sg.copybreak = msg->sg.data[i].length;
>    }
> 
> If we happen to trim the entire msg so size=0 then i==start
> which should mean i < msg->sg.curr unless msg->sg.curr = msg->sg.start
> so we should just use <=. In the second case.
> 
>    else if (msg->sg.end < msg->sg.start &&
>            (i < msg->sg.curr || i > msg->sg.start)) { // i <= msg->sg.curr
>       msg->sg.curr = i;
>       msg->sg.copybreak = msg->sg.data[i].length;
>    }
>  
> If we trim the entire message here i == sg.start again. And same
> thing use <= and we should catch case sg.tart = sg.curr.
> 
> In the full case we didn't trim anything so we shouldn't do any
> manipulating of curr or copybreak.

Hm, don't we need to potentially move the copybreak back a little?
That's why I added this:

if (msg->sg.curr == i && msg->sg.copybreak > msg->sg.data[i].length)
	msg->sg.copybreak = msg->sg.data[i].length;

To make sure that if we trimmed "a little bit" of the last SG but
didn't actually consume it the copybreak doesn't point after the length.
But perhaps that's not needed since sk_msg_memcopy_from_iter() special
cases the copybreak > length, anyway?

> > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> > index e4b3fb4bb77c..ce7055259877 100644
> > --- a/include/linux/skmsg.h
> > +++ b/include/linux/skmsg.h
> > @@ -139,6 +139,11 @@ static inline void sk_msg_apply_bytes(struct sk_psock *psock, u32 bytes)
> >  	}
> >  }
> >  
> > +static inline u32 sk_msg_iter_dist(u32 start, u32 end)
> > +{
> > +	return end >= start ? end - start : end + (MAX_MSG_FRAGS - start);
> > +}
> > +
> >  #define sk_msg_iter_var_prev(var)			\
> >  	do {						\
> >  		if (var == 0)				\
> > @@ -198,9 +203,7 @@ static inline u32 sk_msg_elem_used(const struct sk_msg *msg)
> >  	if (sk_msg_full(msg))
> >  		return MAX_MSG_FRAGS;
> >  
> > -	return msg->sg.end >= msg->sg.start ?
> > -		msg->sg.end - msg->sg.start :
> > -		msg->sg.end + (MAX_MSG_FRAGS - msg->sg.start);
> > +	return sk_msg_iter_dist(msg->sg.start, msg->sg.end);
> >  }
> >  
> >  static inline struct scatterlist *sk_msg_elem(struct sk_msg *msg, int which)
> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> > index cf390e0aa73d..f6b4a70bafa9 100644
> > --- a/net/core/skmsg.c
> > +++ b/net/core/skmsg.c
> > @@ -270,18 +270,26 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
> >  
> >  	msg->sg.data[i].length -= trim;
> >  	sk_mem_uncharge(sk, trim);
> > +	if (msg->sg.curr == i && msg->sg.copybreak > msg->sg.data[i].length)
> > +		msg->sg.copybreak = msg->sg.data[i].length;
> >  out:
> > +	sk_msg_iter_var_next(i);
> > +	msg->sg.end = i;
> > +
> >  	/* If we trim data 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) {  
> 
> I do think its a bit nicer if we don't special case the size = 0 case. If
> we get here and i != start then we would have extra bytes in the sg
> items between the items (i, end) and nonzero size. If i == start then the
> we sg.size = 0. I don't think there are any other cases.

On an empty message i ended up before start, so we'd have to take the
wrapping into account, no? I couldn't come up with a way to handle
that, and the full case cleanly :S Perhaps there are some constraints
on the geometry that simplify it.

> > +		msg->sg.curr = 0;

Ugh, this should say msg->sg.start, not 0.

> > +		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)) {
> > +		sk_msg_iter_var_prev(i);  
> 
> I suspect with small update to dist logic the special case could also
> be dropped here. But I have a preference for my example above at the
> moment. Just getting coffee now so will think on it though.

Oka, I like the dist thing, I thought that's where you were going in
your first email :)

I need to do some more admin, and then I'll probably write a unit test
for this code (use space version).. So we can test either patch with it.

> FWIW I've not compiled my example.
> 
> >  		msg->sg.curr = i;
> >  		msg->sg.copybreak = msg->sg.data[i].length;
> >  	}
> > -	sk_msg_iter_var_next(i);
> > -	msg->sg.end = i;
> >  }
> >  EXPORT_SYMBOL_GPL(sk_msg_trim);