Re: [net] net/tls: Fix to avoid gettig invalid tls record
From: Jakub Kicinski <kuba@kernel.org>
Date: 2020-02-13 04:09:50
Also in:
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?
quoted hunk ↗ jump to hunk
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.