RE: [PATCH net] net/tls: fix sk_msg trim on fallback to copy mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



John Fastabend wrote:
> Jakub Kicinski wrote:
> > sk_msg_trim() tries to only update curr pointer if it falls into
> > the trimmed region. The logic, however, does not take into the
> > account pointer wrapping that sk_msg_iter_var_prev() does.
> > This means that when the message was trimmed completely, the new
> > curr pointer would have the value of MAX_MSG_FRAGS - 1, which is
> > neither smaller than any other value, nor would it actually be
> > correct.
> > 
> > Special case the trimming to 0 length a little bit.
> > 
> > This bug caused the TLS code to not copy all of the message, if
> > zero copy filled in fewer sg entries than memcopy would need.
> > 
> > Big thanks to Alexander Potapenko for the non-KMSAN reproducer.
> > 
> > Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface")
> > Reported-by: syzbot+f8495bff23a879a6d0bd@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Reported-by: syzbot+6f50c99e8f6194bf363f@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
> > ---
> > Daniel, John, does this look okay?
> 
> Thanks for the second ping!
> 
> > 
> > CC: Eric Biggers <ebiggers@xxxxxxxxxx>
> > CC: herbert@xxxxxxxxxxxxxxxxxxx
> > CC: glider@xxxxxxxxxx
> > CC: linux-crypto@xxxxxxxxxxxxxxx
> > 
> >  net/core/skmsg.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> > index cf390e0aa73d..c42c145216b1 100644
> > --- a/net/core/skmsg.c
> > +++ b/net/core/skmsg.c
> > @@ -276,7 +276,10 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
> >  	 * 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 = 0;
> > +		msg->sg.copybreak = 0;
> > +	} else if (msg->sg.curr >= i) {
> >  		msg->sg.curr = i;
> >  		msg->sg.copybreak = msg->sg.data[i].length;
> >  	}
> > -- 
> 
> 
> Its actually not sufficient. We can't directly do comparisons against curr
> like this. msg->sg is a ring buffer so we have to be careful for these
> types of comparisons.
> 
> Examples hopefully help explian. Consider the case with a ring layout on
> entering sk_msg_trim,

Perhaps worth adding this case is only possible AFAIK with BPF manipulating
the ring to buffer/release data.

> 
>    0 1 2                              N = MAX_MSG_FRAGS
>   |_|_|_|...|_|_|_|...|_|_|_|_|....|_|_|
>        ^       ^         ^
>        curr    end       start
> 
> Start trimming from end
> 
>    0 1 2                              N = MAX_MSG_FRAGS
>   |X|X|X|...|X|X|_|...|_|_|i|X|....|X|X|
>        ^       ^         ^
>        curr    end       start
> 
> We trim backwards through ring with sk_msg_iter_var_prev(). And its
> possible to end with the result of above where 'i' is greater than curr
> and greater than start leaving scatterlist elements so size != 0.
> 
>     i > curr && i > start && sg.size != 0
> 
> but we wont catch it with this condition
> 
>     if (msg->sg.curr >= i)
> 
> So we won't reset curr and copybreak so we have a potential issue now
> where curr is pointing at data that has been trimmed.
> 
> I'll put together a fix but the correct thing to do here is a proper
> ring greater than op which is not what we have there. Although, your patch
> is also really a good one to have because reseting curr = 0 and
> copybreak = 0 when possible keeps the ring from being fragmented which
> avoids chaining when we push scatterlists down to crypto layer. So for
> your patch,
> 
> Acked-By: John Fastabend <john.fastabend@xxxxxxxxx>
> 
> If it should go to net or net-next I think is probably up for debate
> 
> Nice catch!!! Can you send me the reproducer?
> 
> Thanks,
> John
> 
> 
> 
> 





[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux