Re: [PATCH net-next v4 1/6] tls: block decryption when a rekey is pending

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 10 Dec 2024 17:16:09 +0100 Sabrina Dubroca wrote:
> > The only reason the check_pending_rekey() can fail is if the message is
> > mis-formatted, I wonder if we are better off ignoring mis-formatted
> > rekeys? User space will see them and break the connection, anyway.
> > Alternatively - we could add a selftest for this.  
> 
> Going back to tls_check_pending_rekey():
> 
> > > +	if (rxm->full_len < 1)
> > > +		return -EINVAL;  
> 
> There's no real reason to fail here, we should probably just ignore
> it. It's not a rekey, and it's not a valid handshake message, but one
> could say that's not the kernel's problem. I'll make that return 0
> unless you want to keep -EINVAL.

returning 0 SGTM

> Hard to write a selftest for because we'd have to do a sendmsg with
> len=0, or do the crypto in the selftest.
> 
> > > +	err = skb_copy_bits(skb, rxm->offset, &hs_type, 1);
> > > +	if (err < 0)
> > > +		return err;  
> 
> This probably means that the skb we got from the parser was broken. If
> we can't read 1B with full_len >= 1, something's wrong. Maybe worth a
> DEBUG_NET_WARN_ON_ONCE?

Also SG!

> > > +	if (hs_type == TLS_HANDSHAKE_KEYUPDATE) {  
> 
> Here I don't actually check if it's a correct KeyUpdate message [1],
> we pause decryption and let userspace decide what to do (probably
> break the connection as you said).