[3/3] net: rxrpc: Replace time_t type with time64_t type
diff mbox

Message ID 8ac57c96bf5a0695ecc67fd230440b0b9d15740f.1502246502.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

(Exiting) Baolin Wang Aug. 9, 2017, 2:51 a.m. UTC
Since the 'expiry' variable of 'struct key_preparsed_payload' has been
changed to 'time64_t' type, which is year 2038 safe on 32bits system.

In net/rxrpc subsystem, we need convert 'u32' type to 'time64_t' type
when copying ticket expires time to 'prep->expiry', then this patch
introduces two helper functions to help convert 'u32' to 'time64_t'
type.

This patch also uses ktime_get_real_seconds() to get current time instead
of get_seconds() which is not year 2038 safe on 32bits system.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/keys/rxrpc-type.h |   21 +++++++++++++++++++++
 net/rxrpc/ar-internal.h   |    2 +-
 net/rxrpc/key.c           |   22 ++++++++++++++--------
 net/rxrpc/rxkad.c         |   14 +++++++-------
 4 files changed, 43 insertions(+), 16 deletions(-)

Comments

Arnd Bergmann Aug. 9, 2017, 9:01 a.m. UTC | #1
On Wed, Aug 9, 2017 at 4:51 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> diff --git a/include/keys/rxrpc-type.h b/include/keys/rxrpc-type.h
> index 5de0673..76421e2 100644
> --- a/include/keys/rxrpc-type.h
> +++ b/include/keys/rxrpc-type.h
> @@ -127,4 +127,25 @@ struct rxrpc_key_data_v1 {
>  #define AFSTOKEN_K5_ADDRESSES_MAX      16      /* max K5 addresses */
>  #define AFSTOKEN_K5_AUTHDATA_MAX       16      /* max K5 pieces of auth data */
>
> +/*
> + * truncate a time64_t to the range from 1970 to 2106 as
> + * in the network protocol
> + */
> +static inline u32 rxrpc_time64_to_u32(time64_t time)
> +{
> +       if (time < 0)
> +               return 0;
> +
> +       if (time > UINT_MAX)
> +               return UINT_MAX;
> +
> +       return (u32)time;
> +}
>+
> +/* extend u32 back to time64_t using the same 1970-2106 range */
> +static inline time64_t rxrpc_u32_to_time64(u32 time)
> +{
> +       return (time64_t)time;
> +}

When reviewing this, I could not find any clear definition on whether
the preparse functions should treat this as a signed or unsigned
32-bit number. The function as defined here documents as an unsigned
as that is more useful (it pushes out the last day this works from year
2038 to 2106) and matches the existing behavior that we got on
64-bit architectures (intentionally or not).

> @@ -433,6 +435,7 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep,
>         struct rxrpc_key_token *token, **pptoken;
>         struct rxk5_key *rxk5;
>         const __be32 *end_xdr = xdr + (toklen >> 2);
> +       time64_t expiry;
>         int ret;
>
>         _enter(",{%x,%x,%x,%x},%u",
> @@ -533,8 +536,9 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep,
>              pptoken = &(*pptoken)->next)
>                 continue;
>         *pptoken = token;
> -       if (token->kad->expiry < prep->expiry)
> -               prep->expiry = token->kad->expiry;
> +       expiry = rxrpc_u32_to_time64(token->kad->expiry);
> +       if (expiry < prep->expiry)
> +               prep->expiry = expiry;
>
>         _leave(" = 0");
>         return 0;

I'm still slightly puzzled by what this function does: it does have
four timestamps
(authtime, starttime, endtime, renew_till) that are all transferred as
64-bit values
and won't expire, but then it also uses the 32-bit expiry field in
rxrpc_key_token->kad->expiry instead of the 64-bit rxrpc_key_token->k5
fields.

This appears to overlay the first 32 bits of the rxrpc_key_token->k5->starttime
field, which is also a time value on little-endian architectures by chance,
but I would assume that it's always in the past, meaning the keys would
already be expired. Any idea what is the intended behavior here?

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells Aug. 9, 2017, 9:33 a.m. UTC | #2
Arnd Bergmann <arnd@arndb.de> wrote:

> > @@ -533,8 +536,9 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep,
> >              pptoken = &(*pptoken)->next)
> >                 continue;
> >         *pptoken = token;
> > -       if (token->kad->expiry < prep->expiry)
> > -               prep->expiry = token->kad->expiry;
> ...
>
> I'm still slightly puzzled by what this function does: it does have four
> timestamps (authtime, starttime, endtime, renew_till) that are all
> transferred as 64-bit values and won't expire, but then it also uses the
> 32-bit expiry field in rxrpc_key_token->kad->expiry instead of the 64-bit
> rxrpc_key_token->k5 fields.

Good catch.  This is a cut'n'paste error.  It should be using
token->k5->expiry here not token->kad->expiry.

> This appears to overlay the first 32 bits of the
> rxrpc_key_token->k5->starttime field, which is also a time value on
> little-endian architectures by chance, but I would assume that it's always
> in the past, meaning the keys would already be expired.

Yeah - I'm not sure why it works.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 9, 2017, 10 a.m. UTC | #3
On Wed, Aug 9, 2017 at 11:33 AM, David Howells <dhowells@redhat.com> wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
>
>> > @@ -533,8 +536,9 @@ static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep,
>> >              pptoken = &(*pptoken)->next)
>> >                 continue;
>> >         *pptoken = token;
>> > -       if (token->kad->expiry < prep->expiry)
>> > -               prep->expiry = token->kad->expiry;
>> ...
>>
>> I'm still slightly puzzled by what this function does: it does have four
>> timestamps (authtime, starttime, endtime, renew_till) that are all
>> transferred as 64-bit values and won't expire, but then it also uses the
>> 32-bit expiry field in rxrpc_key_token->kad->expiry instead of the 64-bit
>> rxrpc_key_token->k5 fields.
>
> Good catch.  This is a cut'n'paste error.  It should be using
> token->k5->expiry here not token->kad->expiry.

Do you know which format is used in practice? Are both kad and k5 common
among rxrpc users?

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells Aug. 9, 2017, 1:26 p.m. UTC | #4
Arnd Bergmann <arnd@arndb.de> wrote:

> Do you know which format is used in practice? Are both kad and k5 common
> among rxrpc users?

The aklog program I'm using uses the non-XDR interface to push a Kerberos 5
ticket to the kernel, so it doesn't actually invoke rxrpc_preparse_xdr() from
rxrpc_preparse().

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 9, 2017, 3:12 p.m. UTC | #5
On Wed, Aug 9, 2017 at 3:26 PM, David Howells <dhowells@redhat.com> wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
>
>> Do you know which format is used in practice? Are both kad and k5 common
>> among rxrpc users?
>
> The aklog program I'm using uses the non-XDR interface to push a Kerberos 5
> ticket to the kernel, so it doesn't actually invoke rxrpc_preparse_xdr() from
> rxrpc_preparse().

Ah, I'm slowly starting to understand how this fits together. So you can add
a key either through key_add() from local user space, or through an rxrpc
socket.

From what I can tell, the program you have at
http://people.redhat.com/~dhowells/rxrpc/klog.c will keep working beyond
2038 but not beyond 2106 on all 64-bit architectures and on those
32-bit systems that have a libc with 64-bit time_t. It could be modified
to use the xdr_rxk5 key format, which would make it use 64-bit time
values (and require the kernel fix mentioned above).

In contrast, the rxrpc socket interface would need a major rework to
support 64-bit expiration times. It receives a kerberos ticket with a
32-bit issue time
that gets used to calculate the expiry time in rxkad_decrypt_ticket, and from
there we pass it through a  rxrpc_key_data_v1 into key_instantiate_and_link(),
which calls rxrpc_preparse() and that just takes the expiry out again and sticks
it into another 32-bit field in struct rxkad_key, from where it
finally gets copied
into the (now 64-bit) key_preparsed_payload->expiry field.

Does my understanding match what you intended for the interfaces? Is there
a need to extend the rxrpc socket interface to support xdr_rxk5 keys as well?

         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells Aug. 9, 2017, 3:45 p.m. UTC | #6
Arnd Bergmann <arnd@arndb.de> wrote:

> Ah, I'm slowly starting to understand how this fits together. So you can add
> a key either through key_add() from local user space, or through an rxrpc
> socket.

No, you can't add keys through an rxrpc socket.

There are three 'classes' of key:

 (1) Client keys (type rxrpc).  These must be added by add_key() by userspace
     (but could also be acquired by upcalling to /sbin/request-key) and then
     the kernel calls request_key() to locate them on entry through either a
     kafs inode/file operation or through sendmsg() to an AF_RXRPC socket.

 (2) Server keys (type rxrpc_s).  These are created by userspace and are
     presented to an AF_RXRPC server socket by calling setsockopt().  The
     server uses these to validate/decrypt the token passed by a RESPONSE
     packet.

 (3) Service connection keys (type rxrpc).  These are created internally by
     AF_RXRPC after a successful challenge/response negotiation to hold the
     security details so that we have a struct key to pass around that
     corresponds to the key in (1).

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/include/keys/rxrpc-type.h b/include/keys/rxrpc-type.h
index 5de0673..76421e2 100644
--- a/include/keys/rxrpc-type.h
+++ b/include/keys/rxrpc-type.h
@@ -127,4 +127,25 @@  struct rxrpc_key_data_v1 {
 #define AFSTOKEN_K5_ADDRESSES_MAX	16	/* max K5 addresses */
 #define AFSTOKEN_K5_AUTHDATA_MAX	16	/* max K5 pieces of auth data */
 
+/*
+ * truncate a time64_t to the range from 1970 to 2106 as
+ * in the network protocol
+ */
+static inline u32 rxrpc_time64_to_u32(time64_t time)
+{
+	if (time < 0)
+		return 0;
+
+	if (time > UINT_MAX)
+		return UINT_MAX;
+
+	return (u32)time;
+}
+
+/* extend u32 back to time64_t using the same 1970-2106 range */
+static inline time64_t rxrpc_u32_to_time64(u32 time)
+{
+	return (time64_t)time;
+}
+
 #endif /* _KEYS_RXRPC_TYPE_H */
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 69b9733..3c11443 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -894,7 +894,7 @@  void rxrpc_new_incoming_connection(struct rxrpc_sock *,
 
 int rxrpc_request_key(struct rxrpc_sock *, char __user *, int);
 int rxrpc_server_keyring(struct rxrpc_sock *, char __user *, int);
-int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time_t,
+int rxrpc_get_server_data_key(struct rxrpc_connection *, const void *, time64_t,
 			      u32);
 
 /*
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 5436922..e2d3661 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -92,6 +92,7 @@  static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep,
 				    const __be32 *xdr, unsigned int toklen)
 {
 	struct rxrpc_key_token *token, **pptoken;
+	time64_t expiry;
 	size_t plen;
 	u32 tktlen;
 
@@ -158,8 +159,9 @@  static int rxrpc_preparse_xdr_rxkad(struct key_preparsed_payload *prep,
 	     pptoken = &(*pptoken)->next)
 		continue;
 	*pptoken = token;
-	if (token->kad->expiry < prep->expiry)
-		prep->expiry = token->kad->expiry;
+	expiry = rxrpc_u32_to_time64(token->kad->expiry);
+	if (expiry < prep->expiry)
+		prep->expiry = expiry;
 
 	_leave(" = 0");
 	return 0;
@@ -433,6 +435,7 @@  static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep,
 	struct rxrpc_key_token *token, **pptoken;
 	struct rxk5_key *rxk5;
 	const __be32 *end_xdr = xdr + (toklen >> 2);
+	time64_t expiry;
 	int ret;
 
 	_enter(",{%x,%x,%x,%x},%u",
@@ -533,8 +536,9 @@  static int rxrpc_preparse_xdr_rxk5(struct key_preparsed_payload *prep,
 	     pptoken = &(*pptoken)->next)
 		continue;
 	*pptoken = token;
-	if (token->kad->expiry < prep->expiry)
-		prep->expiry = token->kad->expiry;
+	expiry = rxrpc_u32_to_time64(token->kad->expiry);
+	if (expiry < prep->expiry)
+		prep->expiry = expiry;
 
 	_leave(" = 0");
 	return 0;
@@ -691,6 +695,7 @@  static int rxrpc_preparse(struct key_preparsed_payload *prep)
 {
 	const struct rxrpc_key_data_v1 *v1;
 	struct rxrpc_key_token *token, **pp;
+	time64_t expiry;
 	size_t plen;
 	u32 kver;
 	int ret;
@@ -777,8 +782,9 @@  static int rxrpc_preparse(struct key_preparsed_payload *prep)
 	while (*pp)
 		pp = &(*pp)->next;
 	*pp = token;
-	if (token->kad->expiry < prep->expiry)
-		prep->expiry = token->kad->expiry;
+	expiry = rxrpc_u32_to_time64(token->kad->expiry);
+	if (expiry < prep->expiry)
+		prep->expiry = expiry;
 	token = NULL;
 	ret = 0;
 
@@ -955,7 +961,7 @@  int rxrpc_server_keyring(struct rxrpc_sock *rx, char __user *optval,
  */
 int rxrpc_get_server_data_key(struct rxrpc_connection *conn,
 			      const void *session_key,
-			      time_t expiry,
+			      time64_t expiry,
 			      u32 kvno)
 {
 	const struct cred *cred = current_cred();
@@ -982,7 +988,7 @@  int rxrpc_get_server_data_key(struct rxrpc_connection *conn,
 	data.kver = 1;
 	data.v1.security_index = RXRPC_SECURITY_RXKAD;
 	data.v1.ticket_length = 0;
-	data.v1.expiry = expiry;
+	data.v1.expiry = rxrpc_time64_to_u32(expiry);
 	data.v1.kvno = 0;
 
 	memcpy(&data.v1.session_key, session_key, sizeof(data.v1.session_key));
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 46d1a1f..34c86d2 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -854,7 +854,7 @@  static int rxkad_decrypt_ticket(struct rxrpc_connection *conn,
 				struct sk_buff *skb,
 				void *ticket, size_t ticket_len,
 				struct rxrpc_crypt *_session_key,
-				time_t *_expiry,
+				time64_t *_expiry,
 				u32 *_abort_code)
 {
 	struct skcipher_request *req;
@@ -864,7 +864,7 @@  static int rxkad_decrypt_ticket(struct rxrpc_connection *conn,
 	struct in_addr addr;
 	unsigned int life;
 	const char *eproto;
-	time_t issue, now;
+	time64_t issue, now;
 	bool little_endian;
 	int ret;
 	u32 abort_code;
@@ -960,15 +960,15 @@  static int rxkad_decrypt_ticket(struct rxrpc_connection *conn,
 	if (little_endian) {
 		__le32 stamp;
 		memcpy(&stamp, p, 4);
-		issue = le32_to_cpu(stamp);
+		issue = rxrpc_u32_to_time64(le32_to_cpu(stamp));
 	} else {
 		__be32 stamp;
 		memcpy(&stamp, p, 4);
-		issue = be32_to_cpu(stamp);
+		issue = rxrpc_u32_to_time64(be32_to_cpu(stamp));
 	}
 	p += 4;
-	now = get_seconds();
-	_debug("KIV ISSUE: %lx [%lx]", issue, now);
+	now = ktime_get_real_seconds();
+	_debug("KIV ISSUE: %llx [%llx]", issue, now);
 
 	/* check the ticket is in date */
 	if (issue > now) {
@@ -1053,7 +1053,7 @@  static int rxkad_verify_response(struct rxrpc_connection *conn,
 	struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
 	struct rxrpc_crypt session_key;
 	const char *eproto;
-	time_t expiry;
+	time64_t expiry;
 	void *ticket;
 	u32 abort_code, version, kvno, ticket_len, level;
 	__be32 csum;