| 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
| ||
|
Date: Thu, 12 Jul 2018 11:14:53 +0000
From: Vakul Garg <vakul.garg@....com>
To: Boris Pismenny <borisp@...lanox.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"davejwatson@...com" <davejwatson@...com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: "aviadye@...lanox.com" <aviadye@...lanox.com>
Subject: RE: [PATCH net-next] net/tls: Removed redundant variable from 'struct
tls_sw_context_rx'
Hi Boris
Thanks for explaining.
Few questions/observations.
1. Isn't ' ctx->decrypted = true' a redundant statement in tls_do_decryption()?
The same has been repeated in tls_recvmsg() after calling decrypt_skb()?
2. Similarly, ctx->saved_data_ready(sk) seems not required in tls_do_decryption().
This is because tls_do_decryption() is already triggered from tls_recvmsg() i.e. from user space app context.
3. In tls_queue(), I think strp->sk->sk_state_change() needs to be replaced with ctx->saved_data_ready().
Regards
Vakul
> -----Original Message-----
> From: Boris Pismenny [mailto:borisp@...lanox.com]
> Sent: Thursday, July 12, 2018 4:11 PM
> To: Vakul Garg <vakul.garg@....com>; davem@...emloft.net;
> davejwatson@...com; netdev@...r.kernel.org
> Cc: aviadye@...lanox.com
> Subject: Re: [PATCH net-next] net/tls: Removed redundant variable from
> 'struct tls_sw_context_rx'
>
> Hi Vakul,
>
> On 7/12/2018 7:03 AM, Vakul Garg wrote:
> > The variable 'decrypted' in 'struct tls_sw_context_rx' is redundant
> > and is being set/unset without purpose. Simplified the code by removing it.
> >
>
> AFAIU, this variable has an important use here. It keeps the state whether
> the current record has been decrypted between invocations of the
> recv/splice system calls. Otherwise, some records would be decrypted more
> than once if the entire record was not read.
>
> > Signed-off-by: Vakul Garg <vakul.garg@....com>
> > ---
> > include/net/tls.h | 1 -
> > net/tls/tls_sw.c | 87 ++++++++++++++++++++++++----------------------------
> ---
> > 2 files changed, 38 insertions(+), 50 deletions(-)
> >
> > diff --git a/include/net/tls.h b/include/net/tls.h index
> > 70c273777fe9..528d0c2d6cc2 100644
> > --- a/include/net/tls.h
> > +++ b/include/net/tls.h
> > @@ -113,7 +113,6 @@ struct tls_sw_context_rx {
> > struct poll_table_struct *wait);
> > struct sk_buff *recv_pkt;
> > u8 control;
> > - bool decrypted;
> >
> > char rx_aad_ciphertext[TLS_AAD_SPACE_SIZE];
> > char rx_aad_plaintext[TLS_AAD_SPACE_SIZE];
> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
> > 0d670c8adf18..e5f2de2c3fd6 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -81,8 +81,6 @@ static int tls_do_decryption(struct sock *sk,
> > rxm->full_len -= tls_ctx->rx.overhead_size;
> > tls_advance_record_sn(sk, &tls_ctx->rx);
> >
> > - ctx->decrypted = true;
> > -
> > ctx->saved_data_ready(sk);
> >
> > out:
> > @@ -756,6 +754,9 @@ int tls_sw_recvmsg(struct sock *sk,
> > bool cmsg = false;
> > int target, err = 0;
> > long timeo;
> > + int page_count;
> > + int to_copy;
> > +
> >
> > flags |= nonblock;
> >
> > @@ -792,46 +793,38 @@ int tls_sw_recvmsg(struct sock *sk,
> > goto recv_end;
> > }
> >
> > - if (!ctx->decrypted) {
> > - int page_count;
> > - int to_copy;
> > -
> > - page_count = iov_iter_npages(&msg->msg_iter,
> > - MAX_SKB_FRAGS);
> > - to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
> > - if (to_copy <= len && page_count < MAX_SKB_FRAGS
> &&
> > - likely(!(flags & MSG_PEEK))) {
> > - struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> > - int pages = 0;
> > -
> > - zc = true;
> > - sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> > - sg_set_buf(&sgin[0], ctx->rx_aad_plaintext,
> > - TLS_AAD_SPACE_SIZE);
> > -
> > - err = zerocopy_from_iter(sk, &msg-
> >msg_iter,
> > - to_copy, &pages,
> > - &chunk, &sgin[1],
> > - MAX_SKB_FRAGS,
> false);
> > - if (err < 0)
> > - goto fallback_to_reg_recv;
> > -
> > - err = decrypt_skb(sk, skb, sgin);
> > - for (; pages > 0; pages--)
> > - put_page(sg_page(&sgin[pages]));
> > - if (err < 0) {
> > - tls_err_abort(sk, EBADMSG);
> > - goto recv_end;
> > - }
> > - } else {
> > + page_count = iov_iter_npages(&msg->msg_iter,
> MAX_SKB_FRAGS);
> > + to_copy = rxm->full_len - tls_ctx->rx.overhead_size;
> > +
> > + if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
> > + likely(!(flags & MSG_PEEK))) {
> > + struct scatterlist sgin[MAX_SKB_FRAGS + 1];
> > + int pages = 0;
> > +
> > + zc = true;
> > + sg_init_table(sgin, MAX_SKB_FRAGS + 1);
> > + sg_set_buf(&sgin[0], ctx->rx_aad_plaintext,
> > + TLS_AAD_SPACE_SIZE);
> > + err = zerocopy_from_iter(sk, &msg->msg_iter,
> to_copy,
> > + &pages, &chunk, &sgin[1],
> > + MAX_SKB_FRAGS, false);
> > + if (err < 0)
> > + goto fallback_to_reg_recv;
> > +
> > + err = decrypt_skb(sk, skb, sgin);
> > + for (; pages > 0; pages--)
> > + put_page(sg_page(&sgin[pages]));
> > + if (err < 0) {
> > + tls_err_abort(sk, EBADMSG);
> > + goto recv_end;
> > + }
> > + } else {
> > fallback_to_reg_recv:
> > - err = decrypt_skb(sk, skb, NULL);
> > - if (err < 0) {
> > - tls_err_abort(sk, EBADMSG);
> > - goto recv_end;
> > - }
> > + err = decrypt_skb(sk, skb, NULL);
> > + if (err < 0) {
> > + tls_err_abort(sk, EBADMSG);
> > + goto recv_end;
> > }
> > - ctx->decrypted = true;
> > }
> >
> > if (!zc) {
> > @@ -895,15 +888,13 @@ ssize_t tls_sw_splice_read(struct socket *sock,
> loff_t *ppos,
> > goto splice_read_end;
> > }
> >
> > - if (!ctx->decrypted) {
> > - err = decrypt_skb(sk, skb, NULL);
> > + err = decrypt_skb(sk, skb, NULL);
> >
> > - if (err < 0) {
> > - tls_err_abort(sk, EBADMSG);
> > - goto splice_read_end;
> > - }
> > - ctx->decrypted = true;
> > + if (err < 0) {
> > + tls_err_abort(sk, EBADMSG);
> > + goto splice_read_end;
> > }
> > +
> > rxm = strp_msg(skb);
> >
> > chunk = min_t(unsigned int, rxm->full_len, len); @@ -998,8 +989,6
> > @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
> > struct tls_context *tls_ctx = tls_get_ctx(strp->sk);
> > struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
> >
> > - ctx->decrypted = false;
> > -
> > ctx->recv_pkt = skb;
> > strp_pause(strp);
> >
> >
Powered by blists - more mailing lists