[alsa-devel] [PATCH] ALSA: timer: fix ioctl compatibility for different data models

[Takashi Iwai]

On Sat, 19 Mar 2016 14:58:21 +0100,
Takashi Sakamoto wrote:
> 
> 'struct snd_timer_gparams' includes some members with 'unsigned long',
> therefore its size differs depending on data models (ILP32/LP64). As a
> result, x86/x32 applications fail to execute ioctl(2) with
> SNDRV_TIMER_GPARAMS on x86_64 machine.
> 
> This commit fixes this bug by adding a pair of structure and ioctl
> command for the compatibility.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> ---
>  sound/core/timer_compat.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
> index 2e90822..5809387 100644
> --- a/sound/core/timer_compat.c
> +++ b/sound/core/timer_compat.c
> @@ -22,6 +22,18 @@
>  
>  #include <linux/compat.h>
>  
> +/*
> + * In LP64, 64 bit storage alignment is used, therefore the size of this
> + * structure is expanded to multiple of 8.

You can't say the 8 bytes alignment as LP64 generic.  It rather
depends on each architecture.  Write something like "64bit storage
alignment may be used (e.g. x86-64)".

> But the size should be aligned to
> + * multiple of 4 for ILP32.

Actually, this isn't guaranteed so but practically seen, all 32bit
architectures on Linux are at most 4 bytes alignment.

> This is a reason to use 'packed' attribute.
> + */
> +struct snd_timer_gparams32 {
> +	struct snd_timer_id tid;
> +	u32 period_num;
> +	u32 period_den;
> +	unsigned char reserved[32];
> +}__attribute__((packed));
> +
>  struct snd_timer_info32 {
>  	u32 flags;
>  	s32 card;
> @@ -32,6 +44,19 @@ struct snd_timer_info32 {
>  	unsigned char reserved[64];
>  };
>  
> +static int snd_timer_user_gparams_compat(struct file *file,
> +					struct snd_timer_gparams32 __user *user)
> +{
> +	struct snd_timer_gparams gparams;
> +
> +	if (copy_from_user(&gparams, user,
> +		sizeof(struct snd_timer_id) + sizeof(u32) + sizeof(u32)))
> +		return -EFAULT;
> +	return snd_timer_user_gparams(file,
> +				(struct snd_timer_gparams __user *)&gparams);

This is not correct in two ways:

- You didn't convert the data to the 64bit struct.  So the passed
  values are garbled.

- Passing the instance on the stack as a user pointer is broken
  (unless you do some hacks).

For the first, you'd need to copy each field one by one.  For the
latter, snd_timer_user_gparams() needs to be split to two version, one
for kernel pointer and one for user pointer with copy_from_user().


thanks,

Takashi