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

[Takashi Sakamoto]

Hi,

On Mar 20 2016 17:01, Takashi Iwai wrote:
> 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.

Indeed. I had misunderstandings...

>> 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().

I was careless for address spaces. Thanks for your indication.

> thanks,
> 
> Takashi


Regards

Takashi Sakamoto