diff mbox series

[PATCHv3,net-next] tls: Return type of non-data records retrieved using MSG_PEEK in recvmsg

Message ID 20190223084010.17021-1-vakul.garg@nxp.com
State Accepted
Delegated to: David Miller
Headers show
Series [PATCHv3,net-next] tls: Return type of non-data records retrieved using MSG_PEEK in recvmsg | expand

Commit Message

Vakul Garg Feb. 23, 2019, 8:42 a.m. UTC 
The patch enables returning 'type' in msghdr for records that are
retrieved with MSG_PEEK in recvmsg. Further it prevents records peeked
from socket from getting clubbed with any other record of different
type when records are subsequently dequeued from strparser.

For each record, we now retain its type in sk_buff's control buffer
cb[]. Inside control buffer, record's full length and offset are already
stored by strparser in 'struct strp_msg'. We store record type after
'struct strp_msg' inside 'struct tls_msg'. For tls1.2, the type is
stored just after record dequeue. For tls1.3, the type is stored after
record has been decrypted.

Inside process_rx_list(), before processing a non-data record, we check
that we must be able to return back the record type to the user
application. If not, the decrypted records in tls context's rx_list is
left there without consuming any data.

Fixes: 692d7b5d1f912 ("tls: Fix recvmsg() to be able to peek across
multiple records")
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---

Changes in v2:
	- Modified 'Fixed:' line to use full commit header line.
Changes in v3:
	- Added a missing ' " ' in Fixed: line.

 include/net/tls.h | 10 +++++++
 net/tls/tls_sw.c  | 78 +++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 77 insertions(+), 11 deletions(-)

Comments

David Miller Feb. 25, 2019, 5:59 a.m. UTC | #1 
From: Vakul Garg <vakul.garg@nxp.com>
Date: Sat, 23 Feb 2019 08:42:37 +0000

> The patch enables returning 'type' in msghdr for records that are
> retrieved with MSG_PEEK in recvmsg. Further it prevents records peeked
> from socket from getting clubbed with any other record of different
> type when records are subsequently dequeued from strparser.
> 
> For each record, we now retain its type in sk_buff's control buffer
> cb[]. Inside control buffer, record's full length and offset are already
> stored by strparser in 'struct strp_msg'. We store record type after
> 'struct strp_msg' inside 'struct tls_msg'. For tls1.2, the type is
> stored just after record dequeue. For tls1.3, the type is stored after
> record has been decrypted.
> 
> Inside process_rx_list(), before processing a non-data record, we check
> that we must be able to return back the record type to the user
> application. If not, the decrypted records in tls context's rx_list is
> left there without consuming any data.
> 
> Fixes: 692d7b5d1f912 ("tls: Fix recvmsg() to be able to peek across
> multiple records")
> Signed-off-by: Vakul Garg <vakul.garg@nxp.com>

Applied, but please don't break up long Fixes: lines into multiple lines.
Always keep it all on one single line.
Vakul Garg Feb. 25, 2019, 6:02 a.m. UTC | #2 
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of David Miller
> Sent: Monday, February 25, 2019 11:29 AM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com; doronrk@fb.com
> Subject: Re: [PATCHv3 net-next] tls: Return type of non-data records
> retrieved using MSG_PEEK in recvmsg
> 
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Sat, 23 Feb 2019 08:42:37 +0000
> 
> > The patch enables returning 'type' in msghdr for records that are
> > retrieved with MSG_PEEK in recvmsg. Further it prevents records peeked
> > from socket from getting clubbed with any other record of different
> > type when records are subsequently dequeued from strparser.
> >
> > For each record, we now retain its type in sk_buff's control buffer
> > cb[]. Inside control buffer, record's full length and offset are
> > already stored by strparser in 'struct strp_msg'. We store record type
> > after 'struct strp_msg' inside 'struct tls_msg'. For tls1.2, the type
> > is stored just after record dequeue. For tls1.3, the type is stored
> > after record has been decrypted.
> >
> > Inside process_rx_list(), before processing a non-data record, we
> > check that we must be able to return back the record type to the user
> > application. If not, the decrypted records in tls context's rx_list is
> > left there without consuming any data.
> >
> > Fixes: 692d7b5d1f912 ("tls: Fix recvmsg() to be able to peek across
> > multiple records")
> > Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
> 
> Applied, but please don't break up long Fixes: lines into multiple lines.
> Always keep it all on one single line.
 
I am sorry.
Since I fixed this problem in v3 patch, did you still find it?
I am confused.
diff mbox series

Patch

diff --git a/include/net/tls.h b/include/net/tls.h
index a8b37226a287..9f4117ae2297 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -129,6 +129,11 @@  struct tls_rec {
 	u8 aead_req_ctx[];
 };
 
+struct tls_msg {
+	struct strp_msg rxm;
+	u8 control;
+};
+
 struct tx_work {
 	struct delayed_work work;
 	struct sock *sk;
@@ -333,6 +338,11 @@  int tls_push_partial_record(struct sock *sk, struct tls_context *ctx,
 int tls_push_pending_closed_record(struct sock *sk, struct tls_context *ctx,
 				   int flags, long *timeo);
 
+static inline struct tls_msg *tls_msg(struct sk_buff *skb)
+{
+	return (struct tls_msg *)strp_msg(skb);
+}
+
 static inline bool tls_is_pending_closed_record(struct tls_context *ctx)
 {
 	return test_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 71be8acfbc9b..1cc830582fa8 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1530,22 +1530,38 @@  static bool tls_sw_advance_skb(struct sock *sk, struct sk_buff *skb,
 }
 
 /* This function traverses the rx_list in tls receive context to copies the
- * decrypted data records into the buffer provided by caller zero copy is not
+ * decrypted records into the buffer provided by caller zero copy is not
  * true. Further, the records are removed from the rx_list if it is not a peek
  * case and the record has been consumed completely.
  */
 static int process_rx_list(struct tls_sw_context_rx *ctx,
 			   struct msghdr *msg,
+			   u8 *control,
+			   bool *cmsg,
 			   size_t skip,
 			   size_t len,
 			   bool zc,
 			   bool is_peek)
 {
 	struct sk_buff *skb = skb_peek(&ctx->rx_list);
+	u8 ctrl = *control;
+	u8 msgc = *cmsg;
+	struct tls_msg *tlm;
 	ssize_t copied = 0;
 
+	/* Set the record type in 'control' if caller didn't pass it */
+	if (!ctrl && skb) {
+		tlm = tls_msg(skb);
+		ctrl = tlm->control;
+	}
+
 	while (skip && skb) {
 		struct strp_msg *rxm = strp_msg(skb);
+		tlm = tls_msg(skb);
+
+		/* Cannot process a record of different type */
+		if (ctrl != tlm->control)
+			return 0;
 
 		if (skip < rxm->full_len)
 			break;
@@ -1559,6 +1575,27 @@  static int process_rx_list(struct tls_sw_context_rx *ctx,
 		struct strp_msg *rxm = strp_msg(skb);
 		int chunk = min_t(unsigned int, rxm->full_len - skip, len);
 
+		tlm = tls_msg(skb);
+
+		/* Cannot process a record of different type */
+		if (ctrl != tlm->control)
+			return 0;
+
+		/* Set record type if not already done. For a non-data record,
+		 * do not proceed if record type could not be copied.
+		 */
+		if (!msgc) {
+			int cerr = put_cmsg(msg, SOL_TLS, TLS_GET_RECORD_TYPE,
+					    sizeof(ctrl), &ctrl);
+			msgc = true;
+			if (ctrl != TLS_RECORD_TYPE_DATA) {
+				if (cerr || msg->msg_flags & MSG_CTRUNC)
+					return -EIO;
+
+				*cmsg = msgc;
+			}
+		}
+
 		if (!zc || (rxm->full_len - skip) > len) {
 			int err = skb_copy_datagram_msg(skb, rxm->offset + skip,
 						    msg, chunk);
@@ -1597,6 +1634,7 @@  static int process_rx_list(struct tls_sw_context_rx *ctx,
 		skb = next_skb;
 	}
 
+	*control = ctrl;
 	return copied;
 }
 
@@ -1614,6 +1652,7 @@  int tls_sw_recvmsg(struct sock *sk,
 	unsigned char control = 0;
 	ssize_t decrypted = 0;
 	struct strp_msg *rxm;
+	struct tls_msg *tlm;
 	struct sk_buff *skb;
 	ssize_t copied = 0;
 	bool cmsg = false;
@@ -1632,7 +1671,8 @@  int tls_sw_recvmsg(struct sock *sk,
 	lock_sock(sk);
 
 	/* Process pending decrypted records. It must be non-zero-copy */
-	err = process_rx_list(ctx, msg, 0, len, false, is_peek);
+	err = process_rx_list(ctx, msg, &control, &cmsg, 0, len, false,
+			      is_peek);
 	if (err < 0) {
 		tls_err_abort(sk, err);
 		goto end;
@@ -1668,6 +1708,12 @@  int tls_sw_recvmsg(struct sock *sk,
 				}
 			}
 			goto recv_end;
+		} else {
+			tlm = tls_msg(skb);
+			if (prot->version == TLS_1_3_VERSION)
+				tlm->control = 0;
+			else
+				tlm->control = ctx->control;
 		}
 
 		rxm = strp_msg(skb);
@@ -1694,22 +1740,34 @@  int tls_sw_recvmsg(struct sock *sk,
 
 		if (err == -EINPROGRESS)
 			num_async++;
+		else if (prot->version == TLS_1_3_VERSION)
+			tlm->control = ctx->control;
+
+		/* If the type of records being processed is not known yet,
+		 * set it to record type just dequeued. If it is already known,
+		 * but does not match the record type just dequeued, go to end.
+		 * We always get record type here since for tls1.2, record type
+		 * is known just after record is dequeued from stream parser.
+		 * For tls1.3, we disable async.
+		 */
+
+		if (!control)
+			control = tlm->control;
+		else if (control != tlm->control)
+			goto recv_end;
 
 		if (!cmsg) {
 			int cerr;
 
 			cerr = put_cmsg(msg, SOL_TLS, TLS_GET_RECORD_TYPE,
-					sizeof(ctx->control), &ctx->control);
+					sizeof(control), &control);
 			cmsg = true;
-			control = ctx->control;
-			if (ctx->control != TLS_RECORD_TYPE_DATA) {
+			if (control != TLS_RECORD_TYPE_DATA) {
 				if (cerr || msg->msg_flags & MSG_CTRUNC) {
 					err = -EIO;
 					goto recv_end;
 				}
 			}
-		} else if (control != ctx->control) {
-			goto recv_end;
 		}
 
 		if (async)
@@ -1784,18 +1842,16 @@  int tls_sw_recvmsg(struct sock *sk,
 
 		/* Drain records from the rx_list & copy if required */
 		if (is_peek || is_kvec)
-			err = process_rx_list(ctx, msg, copied,
+			err = process_rx_list(ctx, msg, &control, &cmsg, copied,
 					      decrypted, false, is_peek);
 		else
-			err = process_rx_list(ctx, msg, 0,
+			err = process_rx_list(ctx, msg, &control, &cmsg, 0,
 					      decrypted, true, is_peek);
 		if (err < 0) {
 			tls_err_abort(sk, err);
 			copied = 0;
 			goto end;
 		}
-
-		WARN_ON(decrypted != err);
 	}
 
 	copied += decrypted;