[PATCH] ALSA: timer: fix gparams ioctl compatibility for different architectures

[PATCH] ALSA: timer: fix gparams ioctl compatibility for different architectures

[Takashi Sakamoto]

'struct snd_timer_gparams' includes some members with 'unsigned long',
therefore its size differs depending on data models of architecture. As
a result, x86/x32 applications fail to execute ioctl(2) with
SNDRV_TIMER_GPARAMS command 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@sakamocchi.jp>
---
 sound/core/timer.c        | 20 +++++++++++++-------
 sound/core/timer_compat.c | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index aa1b15c..ea4d999 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -1502,17 +1502,13 @@ static int snd_timer_user_ginfo(struct file *file,
 	return err;
 }
 
-static int snd_timer_user_gparams(struct file *file,
-				  struct snd_timer_gparams __user *_gparams)
+static int timer_set_gparams(struct snd_timer_gparams *gparams)
 {
-	struct snd_timer_gparams gparams;
 	struct snd_timer *t;
 	int err;
 
-	if (copy_from_user(&gparams, _gparams, sizeof(gparams)))
-		return -EFAULT;
 	mutex_lock(&register_mutex);
-	t = snd_timer_find(&gparams.tid);
+	t = snd_timer_find(&gparams->tid);
 	if (!t) {
 		err = -ENODEV;
 		goto _error;
@@ -1525,12 +1521,22 @@ static int snd_timer_user_gparams(struct file *file,
 		err = -ENOSYS;
 		goto _error;
 	}
-	err = t->hw.set_period(t, gparams.period_num, gparams.period_den);
+	err = t->hw.set_period(t, gparams->period_num, gparams->period_den);
 _error:
 	mutex_unlock(&register_mutex);
 	return err;
 }
 
+static int snd_timer_user_gparams(struct file *file,
+				  struct snd_timer_gparams __user *_gparams)
+{
+	struct snd_timer_gparams gparams;
+
+	if (copy_from_user(&gparams, _gparams, sizeof(gparams)))
+		return -EFAULT;
+	return timer_set_gparams(&gparams);
+}
+
 static int snd_timer_user_gstatus(struct file *file,
 				  struct snd_timer_gstatus __user *_gstatus)
 {
diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
index 2e90822..d10c792 100644
--- a/sound/core/timer_compat.c
+++ b/sound/core/timer_compat.c
@@ -22,6 +22,19 @@
 
 #include <linux/compat.h>
 
+/*
+ * ILP32/LP64 has different size for 'long' type. Additionally, the size
+ * of storage alignment differs depending on architectures. Here, '__packed'
+ * qualifier is used so that the size of this structure is multiple of 4 and
+ * it fits to any architectures with 32 bit storage alignment.
+ */
+struct snd_timer_gparams32 {
+	struct snd_timer_id tid;
+	u32 period_num;
+	u32 period_den;
+	unsigned char reserved[32];
+} __packed;
+
 struct snd_timer_info32 {
 	u32 flags;
 	s32 card;
@@ -32,6 +45,23 @@ 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 (get_user(gparams.tid.dev_class, &user->tid.dev_class) ||
+	    get_user(gparams.tid.dev_sclass, &user->tid.dev_sclass) ||
+	    get_user(gparams.tid.card, &user->tid.card) ||
+	    get_user(gparams.tid.device, &user->tid.device) ||
+	    get_user(gparams.tid.subdevice, &user->tid.subdevice) ||
+	    get_user(gparams.period_num, &user->period_num) ||
+	    get_user(gparams.period_den, &user->period_den))
+		return -EFAULT;
+
+	return timer_set_gparams(&gparams);
+}
+
 static int snd_timer_user_info_compat(struct file *file,
 				      struct snd_timer_info32 __user *_info)
 {
@@ -99,6 +129,7 @@ static int snd_timer_user_status_compat(struct file *file,
  */
 
 enum {
+	SNDRV_TIMER_IOCTL_GPARAMS32 = _IOW('T', 0x04, struct snd_timer_gparams32),
 	SNDRV_TIMER_IOCTL_INFO32 = _IOR('T', 0x11, struct snd_timer_info32),
 	SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
 #ifdef CONFIG_X86_X32
@@ -114,7 +145,6 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
 	case SNDRV_TIMER_IOCTL_PVERSION:
 	case SNDRV_TIMER_IOCTL_TREAD:
 	case SNDRV_TIMER_IOCTL_GINFO:
-	case SNDRV_TIMER_IOCTL_GPARAMS:
 	case SNDRV_TIMER_IOCTL_GSTATUS:
 	case SNDRV_TIMER_IOCTL_SELECT:
 	case SNDRV_TIMER_IOCTL_PARAMS:
@@ -128,6 +158,8 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
 	case SNDRV_TIMER_IOCTL_PAUSE_OLD:
 	case SNDRV_TIMER_IOCTL_NEXT_DEVICE:
 		return snd_timer_user_ioctl(file, cmd, (unsigned long)argp);
+	case SNDRV_TIMER_IOCTL_GPARAMS32:
+		return snd_timer_user_gparams_compat(file, argp);
 	case SNDRV_TIMER_IOCTL_INFO32:
 		return snd_timer_user_info_compat(file, argp);
 	case SNDRV_TIMER_IOCTL_STATUS32:
-- 
2.7.3

Re: [PATCH] ALSA: timer: fix gparams ioctl compatibility for different architectures

[Takashi Iwai]

On Tue, 22 Mar 2016 16:04:06 +0100,
Takashi Sakamoto wrote:
> 
> 'struct snd_timer_gparams' includes some members with 'unsigned long',
> therefore its size differs depending on data models of architecture. As
> a result, x86/x32 applications fail to execute ioctl(2) with
> SNDRV_TIMER_GPARAMS command 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@sakamocchi.jp>
> ---
>  sound/core/timer.c        | 20 +++++++++++++-------
>  sound/core/timer_compat.c | 34 +++++++++++++++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index aa1b15c..ea4d999 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -1502,17 +1502,13 @@ static int snd_timer_user_ginfo(struct file *file,
>  	return err;
>  }
>  
> -static int snd_timer_user_gparams(struct file *file,
> -				  struct snd_timer_gparams __user *_gparams)
> +static int timer_set_gparams(struct snd_timer_gparams *gparams)
>  {
> -	struct snd_timer_gparams gparams;
>  	struct snd_timer *t;
>  	int err;
>  
> -	if (copy_from_user(&gparams, _gparams, sizeof(gparams)))
> -		return -EFAULT;
>  	mutex_lock(&register_mutex);
> -	t = snd_timer_find(&gparams.tid);
> +	t = snd_timer_find(&gparams->tid);
>  	if (!t) {
>  		err = -ENODEV;
>  		goto _error;
> @@ -1525,12 +1521,22 @@ static int snd_timer_user_gparams(struct file *file,
>  		err = -ENOSYS;
>  		goto _error;
>  	}
> -	err = t->hw.set_period(t, gparams.period_num, gparams.period_den);
> +	err = t->hw.set_period(t, gparams->period_num, gparams->period_den);
>  _error:
>  	mutex_unlock(&register_mutex);
>  	return err;
>  }
>  
> +static int snd_timer_user_gparams(struct file *file,
> +				  struct snd_timer_gparams __user *_gparams)
> +{
> +	struct snd_timer_gparams gparams;
> +
> +	if (copy_from_user(&gparams, _gparams, sizeof(gparams)))
> +		return -EFAULT;
> +	return timer_set_gparams(&gparams);
> +}
> +
>  static int snd_timer_user_gstatus(struct file *file,
>  				  struct snd_timer_gstatus __user *_gstatus)
>  {
> diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
> index 2e90822..d10c792 100644
> --- a/sound/core/timer_compat.c
> +++ b/sound/core/timer_compat.c
> @@ -22,6 +22,19 @@
>  
>  #include <linux/compat.h>
>  
> +/*
> + * ILP32/LP64 has different size for 'long' type. Additionally, the size
> + * of storage alignment differs depending on architectures. Here, '__packed'
> + * qualifier is used so that the size of this structure is multiple of 4 and
> + * it fits to any architectures with 32 bit storage alignment.
> + */
> +struct snd_timer_gparams32 {
> +	struct snd_timer_id tid;
> +	u32 period_num;
> +	u32 period_den;
> +	unsigned char reserved[32];
> +} __packed;
> +
>  struct snd_timer_info32 {
>  	u32 flags;
>  	s32 card;
> @@ -32,6 +45,23 @@ 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 (get_user(gparams.tid.dev_class, &user->tid.dev_class) ||
> +	    get_user(gparams.tid.dev_sclass, &user->tid.dev_sclass) ||
> +	    get_user(gparams.tid.card, &user->tid.card) ||
> +	    get_user(gparams.tid.device, &user->tid.device) ||
> +	    get_user(gparams.tid.subdevice, &user->tid.subdevice) ||

struct snd_timer_id is compatible, so you can use just
copy_from_user() for the whole tid, while the rest two fields need to
be copied individually.

Other than that, the patch looks good.


thanks,

Takashi


> +	    get_user(gparams.period_num, &user->period_num) ||
> +	    get_user(gparams.period_den, &user->period_den))
> +		return -EFAULT;
> +
> +	return timer_set_gparams(&gparams);
> +}
> +
>  static int snd_timer_user_info_compat(struct file *file,
>  				      struct snd_timer_info32 __user *_info)
>  {
> @@ -99,6 +129,7 @@ static int snd_timer_user_status_compat(struct file *file,
>   */
>  
>  enum {
> +	SNDRV_TIMER_IOCTL_GPARAMS32 = _IOW('T', 0x04, struct snd_timer_gparams32),
>  	SNDRV_TIMER_IOCTL_INFO32 = _IOR('T', 0x11, struct snd_timer_info32),
>  	SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
>  #ifdef CONFIG_X86_X32
> @@ -114,7 +145,6 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
>  	case SNDRV_TIMER_IOCTL_PVERSION:
>  	case SNDRV_TIMER_IOCTL_TREAD:
>  	case SNDRV_TIMER_IOCTL_GINFO:
> -	case SNDRV_TIMER_IOCTL_GPARAMS:
>  	case SNDRV_TIMER_IOCTL_GSTATUS:
>  	case SNDRV_TIMER_IOCTL_SELECT:
>  	case SNDRV_TIMER_IOCTL_PARAMS:
> @@ -128,6 +158,8 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
>  	case SNDRV_TIMER_IOCTL_PAUSE_OLD:
>  	case SNDRV_TIMER_IOCTL_NEXT_DEVICE:
>  		return snd_timer_user_ioctl(file, cmd, (unsigned long)argp);
> +	case SNDRV_TIMER_IOCTL_GPARAMS32:
> +		return snd_timer_user_gparams_compat(file, argp);
>  	case SNDRV_TIMER_IOCTL_INFO32:
>  		return snd_timer_user_info_compat(file, argp);
>  	case SNDRV_TIMER_IOCTL_STATUS32:
> -- 
> 2.7.3
> 

Re: [PATCH] ALSA: timer: fix gparams ioctl compatibility for different architectures

[Takashi Sakamoto]

Hi,

On Mar 23 2016 00:11, Takashi Iwai wrote:
> On Tue, 22 Mar 2016 16:04:06 +0100,
> Takashi Sakamoto wrote:
>>
>> 'struct snd_timer_gparams' includes some members with 'unsigned long',
>> therefore its size differs depending on data models of architecture. As
>> a result, x86/x32 applications fail to execute ioctl(2) with
>> SNDRV_TIMER_GPARAMS command 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@sakamocchi.jp>
>> ---
>>  sound/core/timer.c        | 20 +++++++++++++-------
>>  sound/core/timer_compat.c | 34 +++++++++++++++++++++++++++++++++-
>>  2 files changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/sound/core/timer.c b/sound/core/timer.c
>> index aa1b15c..ea4d999 100644
>> --- a/sound/core/timer.c
>> +++ b/sound/core/timer.c
>> @@ -1502,17 +1502,13 @@ static int snd_timer_user_ginfo(struct file *file,
>>  	return err;
>>  }
>>  
>> -static int snd_timer_user_gparams(struct file *file,
>> -				  struct snd_timer_gparams __user *_gparams)
>> +static int timer_set_gparams(struct snd_timer_gparams *gparams)
>>  {
>> -	struct snd_timer_gparams gparams;
>>  	struct snd_timer *t;
>>  	int err;
>>  
>> -	if (copy_from_user(&gparams, _gparams, sizeof(gparams)))
>> -		return -EFAULT;
>>  	mutex_lock(&register_mutex);
>> -	t = snd_timer_find(&gparams.tid);
>> +	t = snd_timer_find(&gparams->tid);
>>  	if (!t) {
>>  		err = -ENODEV;
>>  		goto _error;
>> @@ -1525,12 +1521,22 @@ static int snd_timer_user_gparams(struct file *file,
>>  		err = -ENOSYS;
>>  		goto _error;
>>  	}
>> -	err = t->hw.set_period(t, gparams.period_num, gparams.period_den);
>> +	err = t->hw.set_period(t, gparams->period_num, gparams->period_den);
>>  _error:
>>  	mutex_unlock(&register_mutex);
>>  	return err;
>>  }
>>  
>> +static int snd_timer_user_gparams(struct file *file,
>> +				  struct snd_timer_gparams __user *_gparams)
>> +{
>> +	struct snd_timer_gparams gparams;
>> +
>> +	if (copy_from_user(&gparams, _gparams, sizeof(gparams)))
>> +		return -EFAULT;
>> +	return timer_set_gparams(&gparams);
>> +}
>> +
>>  static int snd_timer_user_gstatus(struct file *file,
>>  				  struct snd_timer_gstatus __user *_gstatus)
>>  {
>> diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
>> index 2e90822..d10c792 100644
>> --- a/sound/core/timer_compat.c
>> +++ b/sound/core/timer_compat.c
>> @@ -22,6 +22,19 @@
>>  
>>  #include <linux/compat.h>
>>  
>> +/*
>> + * ILP32/LP64 has different size for 'long' type. Additionally, the size
>> + * of storage alignment differs depending on architectures. Here, '__packed'
>> + * qualifier is used so that the size of this structure is multiple of 4 and
>> + * it fits to any architectures with 32 bit storage alignment.
>> + */
>> +struct snd_timer_gparams32 {
>> +	struct snd_timer_id tid;
>> +	u32 period_num;
>> +	u32 period_den;
>> +	unsigned char reserved[32];
>> +} __packed;
>> +
>>  struct snd_timer_info32 {
>>  	u32 flags;
>>  	s32 card;
>> @@ -32,6 +45,23 @@ 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 (get_user(gparams.tid.dev_class, &user->tid.dev_class) ||
>> +	    get_user(gparams.tid.dev_sclass, &user->tid.dev_sclass) ||
>> +	    get_user(gparams.tid.card, &user->tid.card) ||
>> +	    get_user(gparams.tid.device, &user->tid.device) ||
>> +	    get_user(gparams.tid.subdevice, &user->tid.subdevice) ||
> 
> struct snd_timer_id is compatible, so you can use just
> copy_from_user() for the whole tid, while the rest two fields need to
> be copied individually.
> 
> Other than that, the patch looks good.

OK. I'll post revised version (v3), soon.


Regards

Takashi Sakamoto

> thanks,
> 
> Takashi
> 
> 
>> +	    get_user(gparams.period_num, &user->period_num) ||
>> +	    get_user(gparams.period_den, &user->period_den))
>> +		return -EFAULT;
>> +
>> +	return timer_set_gparams(&gparams);
>> +}
>> +
>>  static int snd_timer_user_info_compat(struct file *file,
>>  				      struct snd_timer_info32 __user *_info)
>>  {
>> @@ -99,6 +129,7 @@ static int snd_timer_user_status_compat(struct file *file,
>>   */
>>  
>>  enum {
>> +	SNDRV_TIMER_IOCTL_GPARAMS32 = _IOW('T', 0x04, struct snd_timer_gparams32),
>>  	SNDRV_TIMER_IOCTL_INFO32 = _IOR('T', 0x11, struct snd_timer_info32),
>>  	SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
>>  #ifdef CONFIG_X86_X32
>> @@ -114,7 +145,6 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
>>  	case SNDRV_TIMER_IOCTL_PVERSION:
>>  	case SNDRV_TIMER_IOCTL_TREAD:
>>  	case SNDRV_TIMER_IOCTL_GINFO:
>> -	case SNDRV_TIMER_IOCTL_GPARAMS:
>>  	case SNDRV_TIMER_IOCTL_GSTATUS:
>>  	case SNDRV_TIMER_IOCTL_SELECT:
>>  	case SNDRV_TIMER_IOCTL_PARAMS:
>> @@ -128,6 +158,8 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
>>  	case SNDRV_TIMER_IOCTL_PAUSE_OLD:
>>  	case SNDRV_TIMER_IOCTL_NEXT_DEVICE:
>>  		return snd_timer_user_ioctl(file, cmd, (unsigned long)argp);
>> +	case SNDRV_TIMER_IOCTL_GPARAMS32:
>> +		return snd_timer_user_gparams_compat(file, argp);
>>  	case SNDRV_TIMER_IOCTL_INFO32:
>>  		return snd_timer_user_info_compat(file, argp);
>>  	case SNDRV_TIMER_IOCTL_STATUS32:
>> -- 
>> 2.7.3