On Wednesday 24 June 2015 18:17:49 Ksenija Stanojevic wrote:
> Replace all instances of time_t with time64_t i.e. change the type used
> for representing time from 32-bit to 64-bit. All 32-bit kernels to date
> use a signed 32-bit time_t type, which can only represent time until
> January 2038.
> The patch also changes the function get_seconds() that returns a 32-bit
> integer to ktime_get_seconds() that returns seconds as 64-bit integer
> and it uses monotonic instead of real time clock.
> Field expiry is changed to u64 in struct rxrpc_key_data_v1 and struct
> rxkad_key and to time64_t in key_preparsed_payload. This change is safe
> since field expiry is used only in this driver.
> Add macro ENCODE32 to export 64bit time to userspace as a 32bit data.
>
> Signed-off-by: Ksenija Stanojevic <[email protected]>
I think it would be better to split the patch into a small series. Changing
all these types is by definition fragile, and I have a hard time following
the logic.
I'd also suggest taking David Howells on Cc here, as he is the primary
author of this code and he may have further thoughts.
I'll try to rearrange the patch a bit in my reply, in order to better
comment on it.
> diff --git a/include/keys/rxrpc-type.h b/include/keys/rxrpc-type.h
> index fc48754..f13b2cb 100644
> --- a/include/keys/rxrpc-type.h
> +++ b/include/keys/rxrpc-type.h
> @@ -27,7 +27,7 @@ extern struct key *rxrpc_get_null_key(const char *);
> struct rxkad_key {
> u32 vice_id;
> u32 start; /* time at which ticket starts */
> - u32 expiry; /* time at which ticket expires */
> + u64 expiry; /* time at which ticket expires */
> u32 kvno; /* key version number */
> u8 primary_flag; /* T if key for primary cell for this
> user */
> u16 ticket_len; /* length of ticket[] */
So the expiry is currently an unsigned 32-bit value in this structure,
presumably
representing time from 1970 to 2106.
> diff --git a/net/rxrpc/ar-key.c b/net/rxrpc/ar-key.c
> index db0f39f..c190cb1 100644
> --- a/net/rxrpc/ar-key.c
> +++ b/net/rxrpc/ar-key.c
> @@ -125,14 +125,14 @@ static int rxrpc_preparse_xdr_rxkad(struct
> key_preparsed_payload *prep,
> token->kad->vice_id = ntohl(xdr[0]);
> token->kad->kvno = ntohl(xdr[1]);
> token->kad->start = ntohl(xdr[4]);
> - token->kad->expiry = ntohl(xdr[5]);
> + token->kad->expiry = (long long)ntohl(xdr[5]);
> token->kad->primary_flag = ntohl(xdr[6]);
> memcpy(&token->kad->session_key, &xdr[2], 8);
> memcpy(&token->kad->ticket, &xdr[8], tktlen);
However, here you convert it to a signed value, which keeps the same
interpretation, but is a little confusing.
> _debug("SCIX: %u", token->security_index);
> _debug("TLEN: %u", token->kad->ticket_len);
> - _debug("EXPY: %x", token->kad->expiry);
> + _debug("EXPY: %lld", token->kad->expiry);
> _debug("KVNO: %u", token->kad->kvno);
> _debug("PRIM: %u", token->kad->primary_flag);
> _debug("SKEY: %02x%02x%02x%02x%02x%02x%02x%02x",
> @@ -727,7 +727,7 @@ static int rxrpc_preparse(struct key_preparsed_payload
> *prep)
>
> _debug("SCIX: %u", v1->security_index);
> _debug("TLEN: %u", v1->ticket_length);
> - _debug("EXPY: %x", v1->expiry);
> + _debug("EXPY: %lld", v1->expiry);
> _debug("KVNO: %u", v1->kvno);
> _debug("SKEY: %02x%02x%02x%02x%02x%02x%02x%02x",
> v1->session_key[0], v1->session_key[1],
Same here, you change from hexadecimal (unsigned by definition) to signed,
but print an unsigned number. Again, the effect is the same, in particular
because the range is known to be limited to an u32, but it's a bit
confusing.
> diff --git a/net/rxrpc/ar-connection.c b/net/rxrpc/ar-connection.c
> index 6631f4f..692b3e6 100644
> --- a/net/rxrpc/ar-connection.c
> +++ b/net/rxrpc/ar-connection.c
> @@ -808,7 +808,7 @@ void rxrpc_put_connection(struct rxrpc_connection *conn)
>
> ASSERTCMP(atomic_read(&conn->usage), >, 0);
>
> - conn->put_time = get_seconds();
> + conn->put_time = ktime_get_seconds();
> if (atomic_dec_and_test(&conn->usage)) {
> _debug("zombie");
> rxrpc_queue_delayed_work(&rxrpc_connection_reap, 0);
> @@ -852,7 +852,7 @@ static void rxrpc_connection_reaper(struct work_struct
> *work)
>
> _enter("");
>
> - now = get_seconds();
> + now = ktime_get_seconds();
> earliest = ULONG_MAX;
>
> write_lock_bh(&rxrpc_connection_lock);
> diff --git a/net/rxrpc/ar-transport.c b/net/rxrpc/ar-transport.c
> index 1976dec..9946467 100644
> --- a/net/rxrpc/ar-transport.c
> +++ b/net/rxrpc/ar-transport.c
> @@ -189,7 +189,7 @@ void rxrpc_put_transport(struct rxrpc_transport *trans)
>
> ASSERTCMP(atomic_read(&trans->usage), >, 0);
>
> - trans->put_time = get_seconds();
> + trans->put_time = ktime_get_seconds();
> if (unlikely(atomic_dec_and_test(&trans->usage))) {
> _debug("zombie");
> /* let the reaper determine the timeout to avoid a race with
> @@ -226,7 +226,7 @@ static void rxrpc_transport_reaper(struct work_struct
> *work)
>
> _enter("");
>
> - now = get_seconds();
> + now = ktime_get_seconds();
> earliest = ULONG_MAX;
>
> /* extract all the transports that have been dead too long */
The put_time changes look much simpler to me than the rest, and this
can easily be split out from the rest of the changes into a separate
self-contained patch, making it clear that those times are never
transferred across the wire.
> diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
> index aef1bd2..24207db 100644
> --- a/net/rxrpc/ar-internal.h
> +++ b/net/rxrpc/ar-internal.h
> @@ -208,7 +208,7 @@ struct rxrpc_transport {
> struct rb_root server_conns; /* server connections on this
> transport */
> struct list_head link; /* link in master session list
> */
> struct sk_buff_head error_queue; /* error packets awaiting
> processing */
> - time_t put_time; /* time at which to reap */
> + time64_t put_time; /* time at which to
> reap */
> spinlock_t client_lock; /* client connection allocation
> lock */
> rwlock_t conn_lock; /* lock for active/dead
> connections */
> atomic_t usage;
> @@ -256,7 +256,7 @@ struct rxrpc_connection {
> struct rxrpc_crypt csum_iv; /* packet checksum base */
> unsigned long events;
> #define RXRPC_CONN_CHALLENGE 0 /* send challenge packet */
> - time_t put_time; /* time at which to reap */
> + time64_t put_time; /* time at which to
> reap */
> rwlock_t lock; /* access lock */
> spinlock_t state_lock; /* state-change lock */
> atomic_t usage;
I'd use "unsigned long" as the type here, matching what you have in
the local variables in rxrpc_connection_reaper, and change the comment
to reflect that this is monotonic time.
We know that a 32-bit number is enough to store a monotonic time value
(because we don't support systems don't get rebooted for 136 years),
but we should also remove the 'time_t' getting mentioned here.
> diff --git a/include/linux/key-type.h b/include/linux/key-type.h
> index ff9f1d3..ff59683 100644
> --- a/include/linux/key-type.h
> +++ b/include/linux/key-type.h
> @@ -45,7 +45,7 @@ struct key_preparsed_payload {
> const void *data; /* Raw data */
> size_t datalen; /* Raw datalen */
> size_t quotalen; /* Quota length for proposed payload */
> - time_t expiry; /* Expiry time of key */
> + time64_t expiry; /* Expiry time of key */
> bool trusted; /* True if key is trusted */
> };
This member is accessed in security/keys/key.c:__key_instantiate_and_link():
if (prep->expiry != TIME_T_MAX) {
key->expiry = prep->expiry;
key_schedule_gc(prep->expiry + key_gc_delay);
}
When we change the type here, we also have to update the code in that
file, especially as TIME_T_MAX can now be a valid timeout in 2038.
The keys/key.c code sets TIME_T_MAX as the default, and out of the
many implementations in the kernel, only the rxrpc one overrides this,
so we should be able to change it to TIME64_T_MAX, but there should
be one patch that takes care of the key_preparsed_payload in both
ends of it, and that patch should explain how you have determined that
all uses of this field are safe to be changed in this manner.
> @@ -101,7 +101,7 @@ struct rxrpc_key_token {
> struct rxrpc_key_data_v1 {
> u16 security_index;
> u16 ticket_length;
> - u32 expiry; /* time_t */
> + u64 expiry; /* time_t */
> u32 kvno;
> u8 session_key[8];
> u8 ticket[0];
The structure matches the binary layout described in the comment
for the rxrpc_preparse() function, I don't think it's safe to change
it here, as we assume that we can simply interpret the data passed
into prep->data using this format.
I can see two options here:
a) redefine the type to explicitly be expressed as 'unsigned 32-bit
seconds' rather than 'time_t' that is signed. This would let us
keep using rxrpc_key_data_v1 until 2106, or add another hack on
top then. If we do this, there should be an explicit way to convert
back and forth between the 'u32 expiry' and a 'time64_t' with
some helper function, ideally one that warns in case of an overflow.
b) define a new rxrpc_key_data_v2 and make the code handle both versions,
so we always store keys as v2 but are able to read both v1 and v2
keys.
> @@ -1134,6 +1134,12 @@ static long rxrpc_read(const struct key *key,
> if (put_user(y, xdr++) < 0) \
> goto fault; \
> } while(0)
> +#define ENCODE32(x) \
> + do { \
> + u32 z = (u32) x; \
> + ENCODE(z); \
> + } while(0)
> +
> #define ENCODE_DATA(l, s) \
> do { \
> u32 _l = (l); \
> @@ -1175,7 +1181,7 @@ static long rxrpc_read(const struct key *key,
> ENCODE(token->kad->kvno);
> ENCODE_DATA(8, token->kad->session_key);
> ENCODE(token->kad->start);
> - ENCODE(token->kad->expiry);
> + ENCODE32(token->kad->expiry);
> ENCODE(token->kad->primary_flag);
> ENCODE_DATA(token->kad->ticket_len, token->kad->ticket);
> break;
Here you have a silent truncation of the expiry value when copying to
user space. You have mentioned in the changelog that you now do this,
but what we really need is either an marker in the code that lets us
get back to this later if it's broken, or an explanation about why we
have concluded that it is safe, and within which constraints that is
true (e.g. defining this to be valid until 2106).
> diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
> index f226709..7c42437 100644
> --- a/net/rxrpc/rxkad.c
> +++ b/net/rxrpc/rxkad.c
> @@ -819,7 +820,7 @@ protocol_error:
> static int rxkad_decrypt_ticket(struct rxrpc_connection *conn,
> void *ticket, size_t ticket_len,
> struct rxrpc_crypt *_session_key,
> - time_t *_expiry,
> + time64_t *_expiry,
> u32 *_abort_code)
> {
> struct blkcipher_desc desc;
> @@ -827,7 +828,7 @@ static int rxkad_decrypt_ticket(struct rxrpc_connection
> *conn,
> struct scatterlist sg[1];
> struct in_addr addr;
> unsigned int life;
> - time_t issue, now;
> + time64_t issue, now;
> bool little_endian;
> int ret;
> u8 *p, *q, *name, *end;
> @@ -915,15 +916,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 = le64_to_cpu((__le64)stamp);
> } else {
> __be32 stamp;
> memcpy(&stamp, p, 4);
> - issue = be32_to_cpu(stamp);
> + issue = be64_to_cpu((__be64)stamp);
> }
This is an incorrect endian conversion: be64_to_cpu((__be64)stamp) will
put the seconds in the upper half of 'issue' variable. The 32-bit time
stamp in the source data is defined as the 'kerberos IV ticket',
and we cannot make that 64-bit.
This means that here we have to change the interpretation of the
32-bit value. This is separate from the rxrpc_key_data_v1 structure,
but we can do the same approach I described above and add a helper
function that turns the 32-bit value into a time64_t, such as this:
static time64_t rxkad_parse_timestamp(void *p, bool little_endian)
{
u32 time32;
if (little_endian) {
__le32 stamp;
memcpy(&stamp, p, 4);
time32 = le32_to_cpu(stamp);
} else {
__be32 stamp;
memcpy(&stamp, p, 4);
time32 = be32_to_cpu(stamp);
}
/*
* The kerberos 4 time value is traditionally a 32-bit time_t, which
* overflows in 2038. By interpreting it as an unsigned value here, we
* gain an extra 68 years until 2106. This works because we know the
* timestamp can never be negative.
*/
return (u64)time32;
}
> p += 4;
> - now = get_seconds();
> - _debug("KIV ISSUE: %lx [%lx]", issue, now);
> + now = ktime_get_seconds();
> + _debug("KIV ISSUE: %lld [%lld]", issue, now);
>
> /* check the ticket is in date */
> if (issue > now) {
Here, we have an incorrect use of ktime_get_seconds(): the 'issue' time
stamp we get here comes from outside, and if we want to compare that
to the current time, we cannot use the monotonic time, but have to use
ktime_get_real_seconds() instead.
Arnd
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
