diff mbox series

[net-next,v1,5/7] net/handshake: Add helpers for parsing incoming TLS Alerts

Message ID 168970685465.5330.12951636644707988195.stgit@oracle-102.nfsv4bat.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series In-kernel support for the TLS Alert protocol | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1344 this patch: 1344
netdev/cc_maintainers warning 1 maintainers not CCed: chuck.lever@oracle.com
netdev/build_clang success Errors and warnings before: 1371 this patch: 1371
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1367 this patch: 1367
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 57 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Chuck Lever July 18, 2023, 7 p.m. UTC 
From: Chuck Lever <chuck.lever@oracle.com>

Kernel TLS consumers can replace common TLS Alert parsing code with
these helpers.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/net/handshake.h |    4 ++++
 net/handshake/alert.c   |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

Comments

Hannes Reinecke July 19, 2023, 7:52 a.m. UTC | #1 
On 7/18/23 21:00, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Kernel TLS consumers can replace common TLS Alert parsing code with
> these helpers.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>   include/net/handshake.h |    4 ++++
>   net/handshake/alert.c   |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+)
> 
> diff --git a/include/net/handshake.h b/include/net/handshake.h
> index bb88dfa6e3c9..d0fd6a3898c6 100644
> --- a/include/net/handshake.h
> +++ b/include/net/handshake.h
> @@ -42,4 +42,8 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
>   bool tls_handshake_cancel(struct sock *sk);
>   void tls_handshake_close(struct socket *sock);
>   
> +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *msg);
> +bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
> +		    u8 *level, u8 *description);
> +
>   #endif /* _NET_HANDSHAKE_H */
> diff --git a/net/handshake/alert.c b/net/handshake/alert.c
> index 999d3ffaf3e3..93e05d8d599c 100644
> --- a/net/handshake/alert.c
> +++ b/net/handshake/alert.c
> @@ -60,3 +60,49 @@ int tls_alert_send(struct socket *sock, u8 level, u8 description)
>   	ret = sock_sendmsg(sock, &msg);
>   	return ret < 0 ? ret : 0;
>   }
> +
> +/**
> + * tls_record_type - Look for TLS RECORD_TYPE information
> + * @sk: socket (for IP address information)
> + * @cmsg: incoming message to be parsed
> + *
> + * Returns zero or a TLS_RECORD_TYPE value.
> + */
> +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *cmsg)
> +{
> +	u8 record_type;
> +
> +	if (cmsg->cmsg_level != SOL_TLS)
> +		return 0;
> +	if (cmsg->cmsg_type != TLS_GET_RECORD_TYPE)
> +		return 0;
> +
> +	record_type = *((u8 *)CMSG_DATA(cmsg));
> +	return record_type;
> +}
> +EXPORT_SYMBOL(tls_record_type);
> +
tls_process_cmsg() does nearly the same thing; why didn't you update it 
to use your helper?

> +/**
> + * tls_alert_recv - Look for TLS Alert messages
> + * @sk: socket (for IP address information)
> + * @msg: incoming message to be parsed
> + * @level: OUT - TLS AlertLevel value
> + * @description: OUT - TLS AlertDescription value
> + *
> + * Return values:
> + *   %true: @msg contained a TLS Alert; @level and @description filled in
> + *   %false: @msg did not contain a TLS Alert
> + */
> +bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
> +		    u8 *level, u8 *description)
> +{
> +	const struct kvec *iov;
> +	u8 *data;
> +
> +	iov = msg->msg_iter.kvec;
> +	data = iov->iov_base;
> +	*level = data[0];
> +	*description = data[1];
> +	return true;
> +}
> +EXPORT_SYMBOL(tls_alert_recv);
> 
Shouldn't we check the type of message here?

Cheers,

Hannes
Chuck Lever III July 19, 2023, 1:36 p.m. UTC | #2 
> On Jul 19, 2023, at 3:52 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 7/18/23 21:00, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>> Kernel TLS consumers can replace common TLS Alert parsing code with
>> these helpers.
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  include/net/handshake.h |    4 ++++
>>  net/handshake/alert.c   |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>> diff --git a/include/net/handshake.h b/include/net/handshake.h
>> index bb88dfa6e3c9..d0fd6a3898c6 100644
>> --- a/include/net/handshake.h
>> +++ b/include/net/handshake.h
>> @@ -42,4 +42,8 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
>>  bool tls_handshake_cancel(struct sock *sk);
>>  void tls_handshake_close(struct socket *sock);
>>  +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *msg);
>> +bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
>> +     u8 *level, u8 *description);
>> +
>>  #endif /* _NET_HANDSHAKE_H */
>> diff --git a/net/handshake/alert.c b/net/handshake/alert.c
>> index 999d3ffaf3e3..93e05d8d599c 100644
>> --- a/net/handshake/alert.c
>> +++ b/net/handshake/alert.c
>> @@ -60,3 +60,49 @@ int tls_alert_send(struct socket *sock, u8 level, u8 description)
>>   ret = sock_sendmsg(sock, &msg);
>>   return ret < 0 ? ret : 0;
>>  }
>> +
>> +/**
>> + * tls_record_type - Look for TLS RECORD_TYPE information
>> + * @sk: socket (for IP address information)
>> + * @cmsg: incoming message to be parsed
>> + *
>> + * Returns zero or a TLS_RECORD_TYPE value.
>> + */
>> +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *cmsg)
>> +{
>> + u8 record_type;
>> +
>> + if (cmsg->cmsg_level != SOL_TLS)
>> + return 0;
>> + if (cmsg->cmsg_type != TLS_GET_RECORD_TYPE)
>> + return 0;
>> +
>> + record_type = *((u8 *)CMSG_DATA(cmsg));
>> + return record_type;
>> +}
>> +EXPORT_SYMBOL(tls_record_type);
>> +
> tls_process_cmsg() does nearly the same thing; why didn't you update it to use your helper?

tls_process_cmsg() is looking for TLS_SET_RECORD_TYPE,
not TLS_GET_RECORD_TYPE. It looks different enough that
I didn't feel comfortable touching it.


>> +/**
>> + * tls_alert_recv - Look for TLS Alert messages
>> + * @sk: socket (for IP address information)
>> + * @msg: incoming message to be parsed
>> + * @level: OUT - TLS AlertLevel value
>> + * @description: OUT - TLS AlertDescription value
>> + *
>> + * Return values:
>> + *   %true: @msg contained a TLS Alert; @level and @description filled in
>> + *   %false: @msg did not contain a TLS Alert
>> + */
>> +bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
>> +     u8 *level, u8 *description)
>> +{
>> + const struct kvec *iov;
>> + u8 *data;
>> +
>> + iov = msg->msg_iter.kvec;
>> + data = iov->iov_base;
>> + *level = data[0];
>> + *description = data[1];
>> + return true;
>> +}
>> +EXPORT_SYMBOL(tls_alert_recv);
> Shouldn't we check the type of message here?

Well it looks like the return value is never used. This
function acts as more of a parser rather than a predicate.
I'll kill the boolean return value.


--
Chuck Lever
Hannes Reinecke July 20, 2023, 5:44 a.m. UTC | #3 
On 7/19/23 15:36, Chuck Lever III wrote:
> 
> 
>> On Jul 19, 2023, at 3:52 AM, Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 7/18/23 21:00, Chuck Lever wrote:
>>> From: Chuck Lever <chuck.lever@oracle.com>
>>> Kernel TLS consumers can replace common TLS Alert parsing code with
>>> these helpers.
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>   include/net/handshake.h |    4 ++++
>>>   net/handshake/alert.c   |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 50 insertions(+)
>>> diff --git a/include/net/handshake.h b/include/net/handshake.h
>>> index bb88dfa6e3c9..d0fd6a3898c6 100644
>>> --- a/include/net/handshake.h
>>> +++ b/include/net/handshake.h
>>> @@ -42,4 +42,8 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
>>>   bool tls_handshake_cancel(struct sock *sk);
>>>   void tls_handshake_close(struct socket *sock);
>>>   +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *msg);
>>> +bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
>>> +     u8 *level, u8 *description);
>>> +
>>>   #endif /* _NET_HANDSHAKE_H */
>>> diff --git a/net/handshake/alert.c b/net/handshake/alert.c
>>> index 999d3ffaf3e3..93e05d8d599c 100644
>>> --- a/net/handshake/alert.c
>>> +++ b/net/handshake/alert.c
>>> @@ -60,3 +60,49 @@ int tls_alert_send(struct socket *sock, u8 level, u8 description)
>>>    ret = sock_sendmsg(sock, &msg);
>>>    return ret < 0 ? ret : 0;
>>>   }
>>> +
>>> +/**
>>> + * tls_record_type - Look for TLS RECORD_TYPE information
>>> + * @sk: socket (for IP address information)
>>> + * @cmsg: incoming message to be parsed
>>> + *
>>> + * Returns zero or a TLS_RECORD_TYPE value.
>>> + */
>>> +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *cmsg)
>>> +{
>>> + u8 record_type;
>>> +
>>> + if (cmsg->cmsg_level != SOL_TLS)
>>> + return 0;
>>> + if (cmsg->cmsg_type != TLS_GET_RECORD_TYPE)
>>> + return 0;
>>> +
>>> + record_type = *((u8 *)CMSG_DATA(cmsg));
>>> + return record_type;
>>> +}
>>> +EXPORT_SYMBOL(tls_record_type);
>>> +
>> tls_process_cmsg() does nearly the same thing; why didn't you update it to use your helper?
> 
> tls_process_cmsg() is looking for TLS_SET_RECORD_TYPE,
> not TLS_GET_RECORD_TYPE. It looks different enough that
> I didn't feel comfortable touching it.
> 
Argl. Totally forgot that we have TLS_GET_RECORD_TYPE and 
TLS_SET_RECORD_TYPE ...
But to make it clear, shouldn't we rather name the function
tls_get_record_type()

Cheers,

Hannes
Chuck Lever III July 20, 2023, 2:22 p.m. UTC | #4 
> On Jul 20, 2023, at 1:44 AM, Hannes Reinecke <hare@suse.de> wrote:
> 
> On 7/19/23 15:36, Chuck Lever III wrote:
>>> On Jul 19, 2023, at 3:52 AM, Hannes Reinecke <hare@suse.de> wrote:
>>> 
>>> On 7/18/23 21:00, Chuck Lever wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>> Kernel TLS consumers can replace common TLS Alert parsing code with
>>>> these helpers.
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>  include/net/handshake.h |    4 ++++
>>>>  net/handshake/alert.c   |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 50 insertions(+)
>>>> diff --git a/include/net/handshake.h b/include/net/handshake.h
>>>> index bb88dfa6e3c9..d0fd6a3898c6 100644
>>>> --- a/include/net/handshake.h
>>>> +++ b/include/net/handshake.h
>>>> @@ -42,4 +42,8 @@ int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
>>>>  bool tls_handshake_cancel(struct sock *sk);
>>>>  void tls_handshake_close(struct socket *sock);
>>>>  +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *msg);
>>>> +bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
>>>> +     u8 *level, u8 *description);
>>>> +
>>>>  #endif /* _NET_HANDSHAKE_H */
>>>> diff --git a/net/handshake/alert.c b/net/handshake/alert.c
>>>> index 999d3ffaf3e3..93e05d8d599c 100644
>>>> --- a/net/handshake/alert.c
>>>> +++ b/net/handshake/alert.c
>>>> @@ -60,3 +60,49 @@ int tls_alert_send(struct socket *sock, u8 level, u8 description)
>>>>   ret = sock_sendmsg(sock, &msg);
>>>>   return ret < 0 ? ret : 0;
>>>>  }
>>>> +
>>>> +/**
>>>> + * tls_record_type - Look for TLS RECORD_TYPE information
>>>> + * @sk: socket (for IP address information)
>>>> + * @cmsg: incoming message to be parsed
>>>> + *
>>>> + * Returns zero or a TLS_RECORD_TYPE value.
>>>> + */
>>>> +u8 tls_record_type(const struct sock *sk, const struct cmsghdr *cmsg)
>>>> +{
>>>> + u8 record_type;
>>>> +
>>>> + if (cmsg->cmsg_level != SOL_TLS)
>>>> + return 0;
>>>> + if (cmsg->cmsg_type != TLS_GET_RECORD_TYPE)
>>>> + return 0;
>>>> +
>>>> + record_type = *((u8 *)CMSG_DATA(cmsg));
>>>> + return record_type;
>>>> +}
>>>> +EXPORT_SYMBOL(tls_record_type);
>>>> +
>>> tls_process_cmsg() does nearly the same thing; why didn't you update it to use your helper?
>> tls_process_cmsg() is looking for TLS_SET_RECORD_TYPE,
>> not TLS_GET_RECORD_TYPE. It looks different enough that
>> I didn't feel comfortable touching it.
> Argl. Totally forgot that we have TLS_GET_RECORD_TYPE and TLS_SET_RECORD_TYPE ...
> But to make it clear, shouldn't we rather name the function
> tls_get_record_type()

Renamed. Thanks for the review! I will post v2 next week, waiting
for more review comments.



--
Chuck Lever
diff mbox series

Patch

diff --git a/include/net/handshake.h b/include/net/handshake.h
index bb88dfa6e3c9..d0fd6a3898c6 100644
--- a/include/net/handshake.h
+++ b/include/net/handshake.h
@@ -42,4 +42,8 @@  int tls_server_hello_psk(const struct tls_handshake_args *args, gfp_t flags);
 bool tls_handshake_cancel(struct sock *sk);
 void tls_handshake_close(struct socket *sock);
 
+u8 tls_record_type(const struct sock *sk, const struct cmsghdr *msg);
+bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
+		    u8 *level, u8 *description);
+
 #endif /* _NET_HANDSHAKE_H */
diff --git a/net/handshake/alert.c b/net/handshake/alert.c
index 999d3ffaf3e3..93e05d8d599c 100644
--- a/net/handshake/alert.c
+++ b/net/handshake/alert.c
@@ -60,3 +60,49 @@  int tls_alert_send(struct socket *sock, u8 level, u8 description)
 	ret = sock_sendmsg(sock, &msg);
 	return ret < 0 ? ret : 0;
 }
+
+/**
+ * tls_record_type - Look for TLS RECORD_TYPE information
+ * @sk: socket (for IP address information)
+ * @cmsg: incoming message to be parsed
+ *
+ * Returns zero or a TLS_RECORD_TYPE value.
+ */
+u8 tls_record_type(const struct sock *sk, const struct cmsghdr *cmsg)
+{
+	u8 record_type;
+
+	if (cmsg->cmsg_level != SOL_TLS)
+		return 0;
+	if (cmsg->cmsg_type != TLS_GET_RECORD_TYPE)
+		return 0;
+
+	record_type = *((u8 *)CMSG_DATA(cmsg));
+	return record_type;
+}
+EXPORT_SYMBOL(tls_record_type);
+
+/**
+ * tls_alert_recv - Look for TLS Alert messages
+ * @sk: socket (for IP address information)
+ * @msg: incoming message to be parsed
+ * @level: OUT - TLS AlertLevel value
+ * @description: OUT - TLS AlertDescription value
+ *
+ * Return values:
+ *   %true: @msg contained a TLS Alert; @level and @description filled in
+ *   %false: @msg did not contain a TLS Alert
+ */
+bool tls_alert_recv(const struct sock *sk, const struct msghdr *msg,
+		    u8 *level, u8 *description)
+{
+	const struct kvec *iov;
+	u8 *data;
+
+	iov = msg->msg_iter.kvec;
+	data = iov->iov_base;
+	*level = data[0];
+	*description = data[1];
+	return true;
+}
+EXPORT_SYMBOL(tls_alert_recv);