| 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
| ||
|
Message-ID: <5612C69C.7020803@hartkopp.net>
Date: Mon, 05 Oct 2015 20:51:08 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Arnd Bergmann <arnd@...db.de>, netdev@...r.kernel.org
CC: y2038@...ts.linaro.org, linux-kernel@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>,
Marc Kleine-Budde <mkl@...gutronix.de>,
linux-can@...r.kernel.org, linux-api@...r.kernel.org
Subject: Re: [PATCH 12/12] [RFC] can: avoid using timeval for uapi
Hello Arnd,
thanks for picking up this y2038 api issue.
On 09/30/2015 01:26 PM, Arnd Bergmann wrote:
> The can subsystem communicates with user space using a bcm_msg_head
> header, which contains two timestamps. This is problematic for
> multiple reasons:
>
> a) The structure layout is currently incompatible between 64-bit
> user space and 32-bit user space, and cannot work in compat
> mode (other than x32).
>
> b) The timeval structure layout will change in 32-bit user
> space when we fix the y2038 overflow problem by redefining
> time_t to 64-bit, making new 32-bit user space incompatible
> with the current kernel interface.
> Cars last a long time and often use old kernels, so the actual
> users of this code are the most likely ones to migrate to y2038
> safe user space.
>
> This tries to work around part of the problem by changing the
> publicly visible user interface in the header, but not the binary
> interface. Fortunately, the values passed around in the structure
> are relative times and do not actually suffer from the y2038
> overflow, so 32-bit is enough here.
>
> We replace the use of 'struct timeval' with a newly defined
> 'struct bcm_timeval' that uses the exact same binary layout
> as before and that still suffers from problem a) but not problem
> b).
>
> The downside of this approach is that any user space program
> that currently assigns a timeval structure to these members
> rather than writing the tv_sec/tv_usec portions individually
> will suffer a compile-time error when built with an updated
> kernel header. Fixing this error makes it work fine with old
> and new headers though.
I double checked some (more) BCM applications I have access to.
E.g. https://github.com/linux-can/can-tests
When you do a 'git grep ival1' there you get something like
tst-bcm-cycle.c: msg.msg_head.ival1.tv_sec = 1;
tst-bcm-cycle.c: msg.msg_head.ival1.tv_usec = 0;
tst-bcm-cycle.c: msg.msg_head.ival1.tv_sec = 0;
tst-bcm-cycle.c: msg.msg_head.ival1.tv_usec = 0;
tst-bcm-dump.c: msg.msg_head.ival1.tv_sec = timeout / 1000000;
tst-bcm-dump.c: msg.msg_head.ival1.tv_usec = timeout % 1000000;
(..)
So the usual way to assign values to ival1 and ival2 is NOT to assign an
existing struct timeval but to directly assign its tv_[u]sec elements.
I applied your bcm.h changes to my local can-tests tree and it compiles
without any problems - as expected. I don't see any serious drawback with your
idea. I wonder whether developers would ever notice this change ...
> We could address problem a) by using '__u32' or 'int' members
> rather than 'long', but that would have a more significant
> downside in also breaking support for all existing 64-bit user
> binaries that might be using this interface, which is likely
> not acceptable.
Indeed.
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> Cc: Oliver Hartkopp <socketcan@...tkopp.net>
Thanks for your good suggestion to make the BCM API y2038 proof!
Acked-by: Oliver Hartkopp <socketcan@...tkopp.net>
> Cc: Marc Kleine-Budde <mkl@...gutronix.de>
> Cc: linux-can@...r.kernel.org
> Cc: linux-api@...r.kernel.org
> ---
> include/uapi/linux/can/bcm.h | 7 ++++++-
> net/can/bcm.c | 15 ++++++++++-----
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/include/uapi/linux/can/bcm.h b/include/uapi/linux/can/bcm.h
> index 89ddb9dc9bdf..7a291dc1ff15 100644
> --- a/include/uapi/linux/can/bcm.h
> +++ b/include/uapi/linux/can/bcm.h
> @@ -47,6 +47,11 @@
> #include <linux/types.h>
> #include <linux/can.h>
>
> +struct bcm_timeval {
> + long tv_sec;
> + long tv_usec;
> +};
> +
> /**
> * struct bcm_msg_head - head of messages to/from the broadcast manager
> * @opcode: opcode, see enum below.
> @@ -62,7 +67,7 @@ struct bcm_msg_head {
> __u32 opcode;
> __u32 flags;
> __u32 count;
> - struct timeval ival1, ival2;
> + struct bcm_timeval ival1, ival2;
> canid_t can_id;
> __u32 nframes;
> struct can_frame frames[0];
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a1ba6875c2a2..6863310d6973 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -96,7 +96,7 @@ struct bcm_op {
> canid_t can_id;
> u32 flags;
> unsigned long frames_abs, frames_filtered;
> - struct timeval ival1, ival2;
> + struct bcm_timeval ival1, ival2;
> struct hrtimer timer, thrtimer;
> struct tasklet_struct tsklet, thrtsklet;
> ktime_t rx_stamp, kt_ival1, kt_ival2, kt_lastmsg;
> @@ -131,6 +131,11 @@ static inline struct bcm_sock *bcm_sk(const struct sock *sk)
> return (struct bcm_sock *)sk;
> }
>
> +static inline ktime_t bcm_timeval_to_ktime(struct bcm_timeval tv)
> +{
> + return ktime_set(tv.tv_sec, tv.tv_usec * NSEC_PER_USEC);
> +}
> +
> #define CFSIZ sizeof(struct can_frame)
> #define OPSIZ sizeof(struct bcm_op)
> #define MHSIZ sizeof(struct bcm_msg_head)
> @@ -953,8 +958,8 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> op->count = msg_head->count;
> op->ival1 = msg_head->ival1;
> op->ival2 = msg_head->ival2;
> - op->kt_ival1 = timeval_to_ktime(msg_head->ival1);
> - op->kt_ival2 = timeval_to_ktime(msg_head->ival2);
> + op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
> + op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
>
> /* disable an active timer due to zero values? */
> if (!op->kt_ival1.tv64 && !op->kt_ival2.tv64)
> @@ -1134,8 +1139,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> /* set timer value */
> op->ival1 = msg_head->ival1;
> op->ival2 = msg_head->ival2;
> - op->kt_ival1 = timeval_to_ktime(msg_head->ival1);
> - op->kt_ival2 = timeval_to_ktime(msg_head->ival2);
> + op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
> + op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
>
> /* disable an active timer due to zero value? */
> if (!op->kt_ival1.tv64)
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists