diff mbox series

[v6,5/8] ALSA: Avoid using timespec for struct snd_rawmidi_status

Message ID 20191112151642.680072-6-arnd@arndb.de
State Superseded
Headers show
Series Fix year 2038 issue for sound subsystem | expand

Commit Message

Arnd Bergmann Nov. 12, 2019, 3:16 p.m. UTC 
From: Baolin Wang <baolin.wang@linaro.org>


The struct snd_rawmidi_status will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Thus we introduced 'struct snd_rawmidi_status32' and 'struct snd_rawmidi_status64'
to handle 32bit time_t and 64bit time_t in native mode, which replace
timespec with s64 type.

In compat mode, we renamed or introduced new structures to handle 32bit/64bit
time_t in compatible mode. The 'struct snd_rawmidi_status32' and
snd_rawmidi_ioctl_status32() are used to handle 32bit time_t in compat mode.
'struct compat_snd_rawmidi_status64' is used to handle 64bit time_t.

When glibc changes time_t to 64-bit, any recompiled program will issue ioctl
commands that the kernel does not understand without this patch.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 include/uapi/sound/asound.h |   3 +
 sound/core/rawmidi.c        | 132 ++++++++++++++++++++++++++++--------
 sound/core/rawmidi_compat.c |  87 ++++++------------------
 3 files changed, 128 insertions(+), 94 deletions(-)

-- 
2.20.0

Comments

Takashi Iwai Nov. 12, 2019, 4:38 p.m. UTC | #1 
On Tue, 12 Nov 2019 16:16:39 +0100,
Arnd Bergmann wrote:
> 

> From: Baolin Wang <baolin.wang@linaro.org>

> 

> The struct snd_rawmidi_status will use 'timespec' type variables to record

> timestamp, which is not year 2038 safe on 32bits system.

> 

> Thus we introduced 'struct snd_rawmidi_status32' and 'struct snd_rawmidi_status64'

> to handle 32bit time_t and 64bit time_t in native mode, which replace

> timespec with s64 type.

> 

> In compat mode, we renamed or introduced new structures to handle 32bit/64bit

> time_t in compatible mode. The 'struct snd_rawmidi_status32' and

> snd_rawmidi_ioctl_status32() are used to handle 32bit time_t in compat mode.

> 'struct compat_snd_rawmidi_status64' is used to handle 64bit time_t.

> 

> When glibc changes time_t to 64-bit, any recompiled program will issue ioctl

> commands that the kernel does not understand without this patch.

> 

> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  include/uapi/sound/asound.h |   3 +

>  sound/core/rawmidi.c        | 132 ++++++++++++++++++++++++++++--------

>  sound/core/rawmidi_compat.c |  87 ++++++------------------

>  3 files changed, 128 insertions(+), 94 deletions(-)

> 

> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h

> index 7b74524f9406..cb830813da5d 100644

> --- a/include/uapi/sound/asound.h

> +++ b/include/uapi/sound/asound.h

> @@ -651,13 +651,16 @@ struct snd_rawmidi_params {

>  	unsigned char reserved[16];	/* reserved for future use */

>  };

>  

> +#ifndef __KERNEL__

>  struct snd_rawmidi_status {

>  	int stream;

> +	unsigned char pad1[sizeof(time_t) - sizeof(int)];

>  	struct timespec tstamp;		/* Timestamp */

>  	size_t avail;			/* available bytes */

>  	size_t xruns;			/* count of overruns since last status (in bytes) */

>  	unsigned char reserved[16];	/* reserved for future use */

>  };


Can we use union instead of padding?  Something like:

struct snd_rawmidi_status {
	union {
		int stream;
		time_t stream_alignment;
	};
  	struct timespec tstamp;		/* Timestamp */
	....


thanks,

Takashi
Arnd Bergmann Nov. 12, 2019, 8:04 p.m. UTC | #2 
On Tue, Nov 12, 2019 at 5:38 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 12 Nov 2019 16:16:39 +0100, Arnd Bergmann wrote:

> > +#ifndef __KERNEL__

> >  struct snd_rawmidi_status {

> >       int stream;

> > +     unsigned char pad1[sizeof(time_t) - sizeof(int)];

> >       struct timespec tstamp;         /* Timestamp */

> >       size_t avail;                   /* available bytes */

> >       size_t xruns;                   /* count of overruns since last status (in bytes) */

> >       unsigned char reserved[16];     /* reserved for future use */

> >  };

>

> Can we use union instead of padding?  Something like:

>

> struct snd_rawmidi_status {

>         union {

>                 int stream;

>                 time_t stream_alignment;

>         };

>         struct timespec tstamp;         /* Timestamp */

>         ....


I think this would work most of the time, though I don't feel this is more
readable than the other version.

More importantly, it requires compiling user applications with GNU extensions
(--std=gnu89 or higher) or C11, but not C99, so this could be a problem
for some applications.

If you feel there is a problem with the padding syntax, how about enclosing
it in a typedef like:

typedef struct { char pad[sizeof(time_t) - sizeof(int)]; } __time_pad;

This typedef could be used in several structures from the other patches
as well.

      Arnd
Takashi Iwai Nov. 12, 2019, 8:26 p.m. UTC | #3 
On Tue, 12 Nov 2019 21:04:10 +0100,
Arnd Bergmann wrote:
> 

> On Tue, Nov 12, 2019 at 5:38 PM Takashi Iwai <tiwai@suse.de> wrote:

> > On Tue, 12 Nov 2019 16:16:39 +0100, Arnd Bergmann wrote:

> > > +#ifndef __KERNEL__

> > >  struct snd_rawmidi_status {

> > >       int stream;

> > > +     unsigned char pad1[sizeof(time_t) - sizeof(int)];

> > >       struct timespec tstamp;         /* Timestamp */

> > >       size_t avail;                   /* available bytes */

> > >       size_t xruns;                   /* count of overruns since last status (in bytes) */

> > >       unsigned char reserved[16];     /* reserved for future use */

> > >  };

> >

> > Can we use union instead of padding?  Something like:

> >

> > struct snd_rawmidi_status {

> >         union {

> >                 int stream;

> >                 time_t stream_alignment;

> >         };

> >         struct timespec tstamp;         /* Timestamp */

> >         ....

> 

> I think this would work most of the time, though I don't feel this is more

> readable than the other version.

> 

> More importantly, it requires compiling user applications with GNU extensions

> (--std=gnu89 or higher) or C11, but not C99, so this could be a problem

> for some applications.


OK, fair enough.

> If you feel there is a problem with the padding syntax, how about enclosing

> it in a typedef like:

> 

> typedef struct { char pad[sizeof(time_t) - sizeof(int)]; } __time_pad;

> 

> This typedef could be used in several structures from the other patches

> as well.


Yes, that improves the readability.


thanks,

Takashi
Arnd Bergmann Nov. 13, 2019, 3:16 p.m. UTC | #4 
On Tue, Nov 12, 2019 at 9:26 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 12 Nov 2019 21:04:10 +0100, Arnd Bergmann wrote:

> >

> > On Tue, Nov 12, 2019 at 5:38 PM Takashi Iwai <tiwai@suse.de> wrote:

> > > On Tue, 12 Nov 2019 16:16:39 +0100, Arnd Bergmann wrote:

> > If you feel there is a problem with the padding syntax, how about enclosing

> > it in a typedef like:

> >

> > typedef struct { char pad[sizeof(time_t) - sizeof(int)]; } __time_pad;

> >

> > This typedef could be used in several structures from the other patches

> > as well.

>

> Yes, that improves the readability.


Ok, added this change to all instances, along with a description in
the changelog
when I add the typedef.

       Arnd
diff mbox series

Patch

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 7b74524f9406..cb830813da5d 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -651,13 +651,16 @@  struct snd_rawmidi_params {
 	unsigned char reserved[16];	/* reserved for future use */
 };
 
+#ifndef __KERNEL__
 struct snd_rawmidi_status {
 	int stream;
+	unsigned char pad1[sizeof(time_t) - sizeof(int)];
 	struct timespec tstamp;		/* Timestamp */
 	size_t avail;			/* available bytes */
 	size_t xruns;			/* count of overruns since last status (in bytes) */
 	unsigned char reserved[16];	/* reserved for future use */
 };
+#endif
 
 #define SNDRV_RAWMIDI_IOCTL_PVERSION	_IOR('W', 0x00, int)
 #define SNDRV_RAWMIDI_IOCTL_INFO	_IOR('W', 0x01, struct snd_rawmidi_info)
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index 8a12a7538d63..cd9bbb546846 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -50,6 +50,29 @@  static DEFINE_MUTEX(register_mutex);
 #define rmidi_dbg(rmidi, fmt, args...) \
 	dev_dbg(&(rmidi)->dev, fmt, ##args)
 
+struct snd_rawmidi_status32 {
+	s32 stream;
+	s32 tstamp_sec;			/* Timestamp */
+	s32 tstamp_nsec;
+	u32 avail;			/* available bytes */
+	u32 xruns;			/* count of overruns since last status (in bytes) */
+	unsigned char reserved[16];	/* reserved for future use */
+};
+
+#define SNDRV_RAWMIDI_IOCTL_STATUS32	_IOWR('W', 0x20, struct snd_rawmidi_status32)
+
+struct snd_rawmidi_status64 {
+	int stream;
+	u8 rsvd[4];			/* alignment */
+	s64 tstamp_sec;			/* Timestamp */
+	s64 tstamp_nsec;
+	size_t avail;			/* available bytes */
+	size_t xruns;			/* count of overruns since last status (in bytes) */
+	unsigned char reserved[16];	/* reserved for future use */
+};
+
+#define SNDRV_RAWMIDI_IOCTL_STATUS64	_IOWR('W', 0x20, struct snd_rawmidi_status64)
+
 static struct snd_rawmidi *snd_rawmidi_search(struct snd_card *card, int device)
 {
 	struct snd_rawmidi *rawmidi;
@@ -677,7 +700,7 @@  int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
 EXPORT_SYMBOL(snd_rawmidi_input_params);
 
 static int snd_rawmidi_output_status(struct snd_rawmidi_substream *substream,
-				     struct snd_rawmidi_status *status)
+				     struct snd_rawmidi_status64 *status)
 {
 	struct snd_rawmidi_runtime *runtime = substream->runtime;
 
@@ -690,7 +713,7 @@  static int snd_rawmidi_output_status(struct snd_rawmidi_substream *substream,
 }
 
 static int snd_rawmidi_input_status(struct snd_rawmidi_substream *substream,
-				    struct snd_rawmidi_status *status)
+				    struct snd_rawmidi_status64 *status)
 {
 	struct snd_rawmidi_runtime *runtime = substream->runtime;
 
@@ -704,6 +727,80 @@  static int snd_rawmidi_input_status(struct snd_rawmidi_substream *substream,
 	return 0;
 }
 
+static int snd_rawmidi_ioctl_status32(struct snd_rawmidi_file *rfile,
+				      struct snd_rawmidi_status32 __user *argp)
+{
+	int err = 0;
+	struct snd_rawmidi_status32 __user *status = argp;
+	struct snd_rawmidi_status32 status32;
+	struct snd_rawmidi_status64 status64;
+
+	if (copy_from_user(&status32, argp,
+			   sizeof(struct snd_rawmidi_status32)))
+		return -EFAULT;
+
+	switch (status32.stream) {
+	case SNDRV_RAWMIDI_STREAM_OUTPUT:
+		if (rfile->output == NULL)
+			return -EINVAL;
+		err = snd_rawmidi_output_status(rfile->output, &status64);
+		break;
+	case SNDRV_RAWMIDI_STREAM_INPUT:
+		if (rfile->input == NULL)
+			return -EINVAL;
+		err = snd_rawmidi_input_status(rfile->input, &status64);
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (err < 0)
+		return err;
+
+	status32 = (struct snd_rawmidi_status32) {
+		.stream = status64.stream,
+		.tstamp_sec = status64.tstamp_sec,
+		.tstamp_nsec = status64.tstamp_nsec,
+		.avail = status64.avail,
+		.xruns = status64.xruns,
+	};
+
+	if (copy_to_user(status, &status32, sizeof(*status)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int snd_rawmidi_ioctl_status64(struct snd_rawmidi_file *rfile,
+				      struct snd_rawmidi_status64 __user *argp)
+{
+	int err = 0;
+	struct snd_rawmidi_status64 status;
+
+	if (copy_from_user(&status, argp, sizeof(struct snd_rawmidi_status64)))
+		return -EFAULT;
+
+	switch (status.stream) {
+	case SNDRV_RAWMIDI_STREAM_OUTPUT:
+		if (rfile->output == NULL)
+			return -EINVAL;
+		err = snd_rawmidi_output_status(rfile->output, &status);
+		break;
+	case SNDRV_RAWMIDI_STREAM_INPUT:
+		if (rfile->input == NULL)
+			return -EINVAL;
+		err = snd_rawmidi_input_status(rfile->input, &status);
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (err < 0)
+		return err;
+	if (copy_to_user(argp, &status,
+			 sizeof(struct snd_rawmidi_status64)))
+		return -EFAULT;
+	return 0;
+}
+
 static long snd_rawmidi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct snd_rawmidi_file *rfile;
@@ -750,33 +847,10 @@  static long snd_rawmidi_ioctl(struct file *file, unsigned int cmd, unsigned long
 			return -EINVAL;
 		}
 	}
-	case SNDRV_RAWMIDI_IOCTL_STATUS:
-	{
-		int err = 0;
-		struct snd_rawmidi_status status;
-
-		if (copy_from_user(&status, argp, sizeof(struct snd_rawmidi_status)))
-			return -EFAULT;
-		switch (status.stream) {
-		case SNDRV_RAWMIDI_STREAM_OUTPUT:
-			if (rfile->output == NULL)
-				return -EINVAL;
-			err = snd_rawmidi_output_status(rfile->output, &status);
-			break;
-		case SNDRV_RAWMIDI_STREAM_INPUT:
-			if (rfile->input == NULL)
-				return -EINVAL;
-			err = snd_rawmidi_input_status(rfile->input, &status);
-			break;
-		default:
-			return -EINVAL;
-		}
-		if (err < 0)
-			return err;
-		if (copy_to_user(argp, &status, sizeof(struct snd_rawmidi_status)))
-			return -EFAULT;
-		return 0;
-	}
+	case SNDRV_RAWMIDI_IOCTL_STATUS32:
+		return snd_rawmidi_ioctl_status32(rfile, argp);
+	case SNDRV_RAWMIDI_IOCTL_STATUS64:
+		return snd_rawmidi_ioctl_status64(rfile, argp);
 	case SNDRV_RAWMIDI_IOCTL_DROP:
 	{
 		int val;
diff --git a/sound/core/rawmidi_compat.c b/sound/core/rawmidi_compat.c
index 66eee61674b6..7397130976d0 100644
--- a/sound/core/rawmidi_compat.c
+++ b/sound/core/rawmidi_compat.c
@@ -41,19 +41,22 @@  static int snd_rawmidi_ioctl_params_compat(struct snd_rawmidi_file *rfile,
 	return -EINVAL;
 }
 
-struct snd_rawmidi_status32 {
+struct compat_snd_rawmidi_status64 {
 	s32 stream;
-	struct compat_timespec tstamp;
+	u8 rsvd[4]; /* alignment */
+	s64 tstamp_sec;
+	s64 tstamp_nsec;
 	u32 avail;
 	u32 xruns;
 	unsigned char reserved[16];
 } __attribute__((packed));
 
-static int snd_rawmidi_ioctl_status_compat(struct snd_rawmidi_file *rfile,
-					   struct snd_rawmidi_status32 __user *src)
+static int snd_rawmidi_ioctl_status_compat64(struct snd_rawmidi_file *rfile,
+					     struct compat_snd_rawmidi_status64 __user *src)
 {
 	int err;
-	struct snd_rawmidi_status status;
+	struct snd_rawmidi_status64 status;
+	struct compat_snd_rawmidi_status64 compat_status;
 
 	if (get_user(status.stream, &src->stream))
 		return -EFAULT;
@@ -75,68 +78,24 @@  static int snd_rawmidi_ioctl_status_compat(struct snd_rawmidi_file *rfile,
 	if (err < 0)
 		return err;
 
-	if (compat_put_timespec(&status.tstamp, &src->tstamp) ||
-	    put_user(status.avail, &src->avail) ||
-	    put_user(status.xruns, &src->xruns))
-		return -EFAULT;
-
-	return 0;
-}
-
-#ifdef CONFIG_X86_X32
-/* X32 ABI has 64bit timespec and 64bit alignment */
-struct snd_rawmidi_status_x32 {
-	s32 stream;
-	u32 rsvd; /* alignment */
-	struct timespec tstamp;
-	u32 avail;
-	u32 xruns;
-	unsigned char reserved[16];
-} __attribute__((packed));
-
-#define put_timespec(src, dst) copy_to_user(dst, src, sizeof(*dst))
-
-static int snd_rawmidi_ioctl_status_x32(struct snd_rawmidi_file *rfile,
-					struct snd_rawmidi_status_x32 __user *src)
-{
-	int err;
-	struct snd_rawmidi_status status;
-
-	if (get_user(status.stream, &src->stream))
-		return -EFAULT;
-
-	switch (status.stream) {
-	case SNDRV_RAWMIDI_STREAM_OUTPUT:
-		if (!rfile->output)
-			return -EINVAL;
-		err = snd_rawmidi_output_status(rfile->output, &status);
-		break;
-	case SNDRV_RAWMIDI_STREAM_INPUT:
-		if (!rfile->input)
-			return -EINVAL;
-		err = snd_rawmidi_input_status(rfile->input, &status);
-		break;
-	default:
-		return -EINVAL;
-	}
-	if (err < 0)
-		return err;
+	compat_status = (struct compat_snd_rawmidi_status64) {
+		.stream = status.stream,
+		.tstamp_sec = status.tstamp_sec,
+		.tstamp_nsec = status.tstamp_nsec,
+		.avail = status.avail,
+		.xruns = status.xruns,
+	};
 
-	if (put_timespec(&status.tstamp, &src->tstamp) ||
-	    put_user(status.avail, &src->avail) ||
-	    put_user(status.xruns, &src->xruns))
+	if (copy_to_user(src, &compat_status, sizeof(*src)))
 		return -EFAULT;
 
 	return 0;
 }
-#endif /* CONFIG_X86_X32 */
 
 enum {
 	SNDRV_RAWMIDI_IOCTL_PARAMS32 = _IOWR('W', 0x10, struct snd_rawmidi_params32),
-	SNDRV_RAWMIDI_IOCTL_STATUS32 = _IOWR('W', 0x20, struct snd_rawmidi_status32),
-#ifdef CONFIG_X86_X32
-	SNDRV_RAWMIDI_IOCTL_STATUS_X32 = _IOWR('W', 0x20, struct snd_rawmidi_status_x32),
-#endif /* CONFIG_X86_X32 */
+	SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT32 = _IOWR('W', 0x20, struct snd_rawmidi_status32),
+	SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT64 = _IOWR('W', 0x20, struct compat_snd_rawmidi_status64),
 };
 
 static long snd_rawmidi_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
@@ -153,12 +112,10 @@  static long snd_rawmidi_ioctl_compat(struct file *file, unsigned int cmd, unsign
 		return snd_rawmidi_ioctl(file, cmd, (unsigned long)argp);
 	case SNDRV_RAWMIDI_IOCTL_PARAMS32:
 		return snd_rawmidi_ioctl_params_compat(rfile, argp);
-	case SNDRV_RAWMIDI_IOCTL_STATUS32:
-		return snd_rawmidi_ioctl_status_compat(rfile, argp);
-#ifdef CONFIG_X86_X32
-	case SNDRV_RAWMIDI_IOCTL_STATUS_X32:
-		return snd_rawmidi_ioctl_status_x32(rfile, argp);
-#endif /* CONFIG_X86_X32 */
+	case SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT32:
+		return snd_rawmidi_ioctl_status32(rfile, argp);
+	case SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT64:
+		return snd_rawmidi_ioctl_status_compat64(rfile, argp);
 	}
 	return -ENOIOCTLCMD;
 }