All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v4] net/tls: Fix to avoid gettig invalid tls record
@ 2020-02-19  4:10 Rohit Maheshwari
  2020-02-19 19:28 ` Jakub Kicinski
  2020-02-20  0:32 ` David Miller
  0 siblings, 2 replies; 3+ messages in thread
From: Rohit Maheshwari @ 2020-02-19  4:10 UTC (permalink / raw)
  To: davem, kuba, netdev
  Cc: borisp, aviadye, john.fastabend, daniel, manojmalviya, Rohit Maheshwari

Current code doesn't check if tcp sequence number is starting from (/after)
1st record's start sequnce number. It only checks if seq number is before
1st record's end sequnce number. This problem will always be a possibility
in re-transmit case. If a record which belongs to a requested seq number is
already deleted, tls_get_record will start looking into list and as per the
check it will look if seq number is before the end seq of 1st record, which
will always be true and will return 1st record always, it should in fact
return NULL.
As part of the fix, start looking each record only if the sequence number
lies in the list else return NULL.
There is one more check added, driver look for the start marker record to
handle tcp packets which are before the tls offload start sequence number,
hence return 1st record if the record is tls start marker and seq number is
before the 1st record's starting sequence number.

Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
---
 net/tls/tls_device.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index cd91ad812291..e72d7d787935 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -592,7 +592,7 @@ struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
 				       u32 seq, u64 *p_record_sn)
 {
 	u64 record_sn = context->hint_record_sn;
-	struct tls_record_info *info;
+	struct tls_record_info *info, *last;
 
 	info = context->retransmit_hint;
 	if (!info ||
@@ -604,6 +604,24 @@ struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
 						struct tls_record_info, list);
 		if (!info)
 			return NULL;
+		/* send the start_marker record if seq number is before the
+		 * tls offload start marker sequence number. This record is
+		 * required to handle TCP packets which are before TLS offload
+		 * started.
+		 *  And if it's not start marker, look if this seq number
+		 * belongs to the list.
+		 */
+		if (likely(!tls_record_is_start_marker(info))) {
+			/* we have the first record, get the last record to see
+			 * if this seq number belongs to the list.
+			 */
+			last = list_last_entry(&context->records_list,
+					       struct tls_record_info, list);
+
+			if (!between(seq, tls_record_start_seq(info),
+				     last->end_seq))
+				return NULL;
+		}
 		record_sn = context->unacked_record_sn;
 	}
 
-- 
2.25.0.191.gde93cc1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH net v4] net/tls: Fix to avoid gettig invalid tls record
  2020-02-19  4:10 [PATCH net v4] net/tls: Fix to avoid gettig invalid tls record Rohit Maheshwari
@ 2020-02-19 19:28 ` Jakub Kicinski
  2020-02-20  0:32 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2020-02-19 19:28 UTC (permalink / raw)
  To: Rohit Maheshwari
  Cc: davem, netdev, borisp, aviadye, john.fastabend, daniel, manojmalviya

On Wed, 19 Feb 2020 09:40:22 +0530 Rohit Maheshwari wrote:
> Current code doesn't check if tcp sequence number is starting from (/after)
> 1st record's start sequnce number. It only checks if seq number is before
> 1st record's end sequnce number. This problem will always be a possibility
> in re-transmit case. If a record which belongs to a requested seq number is
> already deleted, tls_get_record will start looking into list and as per the
> check it will look if seq number is before the end seq of 1st record, which
> will always be true and will return 1st record always, it should in fact
> return NULL.
> As part of the fix, start looking each record only if the sequence number
> lies in the list else return NULL.
> There is one more check added, driver look for the start marker record to
> handle tcp packets which are before the tls offload start sequence number,
> hence return 1st record if the record is tls start marker and seq number is
> before the 1st record's starting sequence number.
> 
> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

FWIW you could have kept the review tag from Boris, the change to v4 
was minor.

Thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net v4] net/tls: Fix to avoid gettig invalid tls record
  2020-02-19  4:10 [PATCH net v4] net/tls: Fix to avoid gettig invalid tls record Rohit Maheshwari
  2020-02-19 19:28 ` Jakub Kicinski
@ 2020-02-20  0:32 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2020-02-20  0:32 UTC (permalink / raw)
  To: rohitm
  Cc: kuba, netdev, borisp, aviadye, john.fastabend, daniel, manojmalviya

From: Rohit Maheshwari <rohitm@chelsio.com>
Date: Wed, 19 Feb 2020 09:40:22 +0530

> Current code doesn't check if tcp sequence number is starting from (/after)
> 1st record's start sequnce number. It only checks if seq number is before
> 1st record's end sequnce number. This problem will always be a possibility
> in re-transmit case. If a record which belongs to a requested seq number is
> already deleted, tls_get_record will start looking into list and as per the
> check it will look if seq number is before the end seq of 1st record, which
> will always be true and will return 1st record always, it should in fact
> return NULL.
> As part of the fix, start looking each record only if the sequence number
> lies in the list else return NULL.
> There is one more check added, driver look for the start marker record to
> handle tcp packets which are before the tls offload start sequence number,
> hence return 1st record if the record is tls start marker and seq number is
> before the 1st record's starting sequence number.
> 
> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>

Applied and queued up for -stable.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-02-20  0:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-19  4:10 [PATCH net v4] net/tls: Fix to avoid gettig invalid tls record Rohit Maheshwari
2020-02-19 19:28 ` Jakub Kicinski
2020-02-20  0:32 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.