Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
* [net] net/tls: Fix to avoid gettig invalid tls record
@ 2020-02-12  7:16 Rohit Maheshwari
  2020-02-13  4:09 ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Rohit Maheshwari @ 2020-02-12  7:16 UTC (permalink / raw)
  To: davem, netdev; +Cc: linux-crypto, 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. 

Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
---
 net/tls/tls_device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index cd91ad812291..2898517298bf 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -602,7 +602,8 @@ struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
 		 */
 		info = list_first_entry_or_null(&context->records_list,
 						struct tls_record_info, list);
-		if (!info)
+		/* return NULL if seq number even before the 1st entry. */
+		if (!info || before(seq, info->end_seq - info->len))
 			return NULL;
 		record_sn = context->unacked_record_sn;
 	}
-- 
2.25.0.191.gde93cc1


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

* Re: [net] net/tls: Fix to avoid gettig invalid tls record
  2020-02-12  7:16 [net] net/tls: Fix to avoid gettig invalid tls record Rohit Maheshwari
@ 2020-02-13  4:09 ` Jakub Kicinski
  2020-02-13  6:55   ` rohit maheshwari
  0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2020-02-13  4:09 UTC (permalink / raw)
  To: Rohit Maheshwari; +Cc: davem, netdev, linux-crypto

On Wed, 12 Feb 2020 12:46:30 +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. 

I think I see your point, do you observe this problem in practice 
or did you find this through code review?

> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index cd91ad812291..2898517298bf 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -602,7 +602,8 @@ struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
>  		 */
>  		info = list_first_entry_or_null(&context->records_list,
>  						struct tls_record_info, list);
> -		if (!info)
> +		/* return NULL if seq number even before the 1st entry. */
> +		if (!info || before(seq, info->end_seq - info->len))

Is it not more appropriate to use between() in the actual comparison
below? I feel like with this patch we can get false negatives.

>  			return NULL;
>  		record_sn = context->unacked_record_sn;
>  	}

If you post a v2 please add a Fixes tag and CC maintainers of this code.

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

* Re: [net] net/tls: Fix to avoid gettig invalid tls record
  2020-02-13  4:09 ` Jakub Kicinski
@ 2020-02-13  6:55   ` rohit maheshwari
  2020-02-13 15:30     ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: rohit maheshwari @ 2020-02-13  6:55 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, netdev, linux-crypto


On 13/02/20 9:39 AM, Jakub Kicinski wrote:
> On Wed, 12 Feb 2020 12:46:30 +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.
> I think I see your point, do you observe this problem in practice
> or did you find this through code review?
I am seeing this issue while running stress test.
>> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
>> index cd91ad812291..2898517298bf 100644
>> --- a/net/tls/tls_device.c
>> +++ b/net/tls/tls_device.c
>> @@ -602,7 +602,8 @@ struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
>>   		 */
>>   		info = list_first_entry_or_null(&context->records_list,
>>   						struct tls_record_info, list);
>> -		if (!info)
>> +		/* return NULL if seq number even before the 1st entry. */
>> +		if (!info || before(seq, info->end_seq - info->len))
> Is it not more appropriate to use between() in the actual comparison
> below? I feel like with this patch we can get false negatives.

If we use between(), though record doesn't exist, we still go and 
compare each record,

which I think, should actually be avoided.

>>   			return NULL;
>>   		record_sn = context->unacked_record_sn;
>>   	}
> If you post a v2 please add a Fixes tag and CC maintainers of this code.

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

* Re: [net] net/tls: Fix to avoid gettig invalid tls record
  2020-02-13  6:55   ` rohit maheshwari
@ 2020-02-13 15:30     ` Jakub Kicinski
  0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2020-02-13 15:30 UTC (permalink / raw)
  To: rohit maheshwari; +Cc: davem, netdev, linux-crypto

On Thu, 13 Feb 2020 12:25:36 +0530 rohit maheshwari wrote:
> On 13/02/20 9:39 AM, Jakub Kicinski wrote:
> >> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> >> index cd91ad812291..2898517298bf 100644
> >> --- a/net/tls/tls_device.c
> >> +++ b/net/tls/tls_device.c
> >> @@ -602,7 +602,8 @@ struct tls_record_info *tls_get_record(struct
> >> tls_offload_context_tx *context, */
> >>   		info =
> >> list_first_entry_or_null(&context->records_list, struct
> >> tls_record_info, list);
> >> -		if (!info)
> >> +		/* return NULL if seq number even before the 1st
> >> entry. */
> >> +		if (!info || before(seq, info->end_seq -
> >> info->len))  
> > Is it not more appropriate to use between() in the actual comparison
> > below? I feel like with this patch we can get false negatives.  
> 
> If we use between(), though record doesn't exist, we still go and 
> compare each record,
> 
> which I think, should actually be avoided.

You can between() first and last element on the list at the very start 
of the search.

> >>   			return NULL;
> >>   		record_sn = context->unacked_record_sn;
> >>   	}  
> > If you post a v2 please add a Fixes tag and CC maintainers of this
> > code.  


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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12  7:16 [net] net/tls: Fix to avoid gettig invalid tls record Rohit Maheshwari
2020-02-13  4:09 ` Jakub Kicinski
2020-02-13  6:55   ` rohit maheshwari
2020-02-13 15:30     ` Jakub Kicinski

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git