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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <74209F42-2099-4682-9478-54040B1BCC1A@oracle.com>
Date: Wed, 19 Jul 2023 13:36:10 +0000
From: Chuck Lever III <chuck.lever@...cle.com>
To: Hannes Reinecke <hare@...e.de>
CC: Chuck Lever <cel@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org"
	<kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kernel-tls-handshake@...ts.linux.dev" <kernel-tls-handshake@...ts.linux.dev>
Subject: Re: [PATCH net-next v1 5/7] net/handshake: Add helpers for parsing
 incoming TLS Alerts



> On Jul 19, 2023, at 3:52 AM, Hannes Reinecke <hare@...e.de> wrote:
> 
> On 7/18/23 21:00, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@...cle.com>
>> Kernel TLS consumers can replace common TLS Alert parsing code with
>> these helpers.
>> Signed-off-by: Chuck Lever <chuck.lever@...cle.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