[PATCH net v2] net/tls: Fix to avoid gettig invalid tls record

[PATCH net v2] net/tls: Fix to avoid gettig invalid tls record

[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 checking if the sequence number lies in the list else
return NULL.

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

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index cd91ad812291..3ee06759dc80 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,15 @@ struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
 						struct tls_record_info, list);
 		if (!info)
 			return NULL;
+		/* we have 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);
+		WARN_ON(!last);
+
+		if (!between(seq, tls_record_start_seq(info), last->end_seq))
+			return NULL;
 		record_sn = context->unacked_record_sn;
 	}
 
-- 
2.25.0.191.gde93cc1

Re: [PATCH net v2] net/tls: Fix to avoid gettig invalid tls record

[Boris Pismenny]

Hi Rohit,

On 14/02/2020 11:14, 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 checking if the sequence number lies in the list else
> return NULL.
> 
> Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure")
> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
> ---
>  net/tls/tls_device.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index cd91ad812291..3ee06759dc80 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,15 @@ struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
>  						struct tls_record_info, list);
>  		if (!info)
>  			return NULL;
> +		/* we have 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);
> +		WARN_ON(!last);
> +
> +		if (!between(seq, tls_record_start_seq(info), last->end_seq))
> +			return NULL;

There's one case in which this is problematic:
TCP packets sent before TLS offload has started. In this case, we use
the start_marker_record (end_seq=first_offload_seq, len=0).
The record_info of the start marker is returned by tls_get_record, and
the driver can handle it properly. However, this patch breaks this,
as the code above returns NULL for these packets which implies that
these packets have been acked and should be dropped by the driver. But,
these packets should be transmitted as they are with no offload.

Adding an unlikely check for the start marker would resolve this issue.