[alsa-devel] [PATCH] ALSA: timer: fix another ioctl compatibility bug
Hi,
In ALSA timer interface, I found another compatibility bug to different ABIs. This patch is to fix it. I note that the bug is in really, really-minor functionality and no drivers use it, therefore it has little impact to this subsystem. This patch is just for manners between kernel/userspace interfaces.
To check the compatibility, I wrote a series of easy and small programs for each ALSA interface. It's available in my repository on github. https://github.com/takaswie/alsa-ioctl-test
As long as I tested with these programs, this patch is the last fix for the compatibility issues which comes from differences of data model. On the other hand, I confirmed that each compatibility layer is not fully compatible to core implementation. For example, ioctl() with SNDRV_PCM_IOCTL_WRITEI_FRAMES and argument filled by zero for PCM capture character device (not for playback device) returns different error codes for each of x86/x86_64 binary. I think this should be fixed, but don't start to work for it yet.
Regards
Takashi Sakamoto (1): ALSA: timer: fix ioctl compatibility for different data models
sound/core/timer_compat.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)
'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@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. But the size should be aligned to + * multiple of 4 for ILP32. 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); +} + static int snd_timer_user_info_compat(struct file *file, struct snd_timer_info32 __user *_info) { @@ -99,6 +124,7 @@ static int snd_timer_user_status_compat(struct file *file, */
enum { + SNDRV_TIMER_IOCTL_GPARAMS32 = _IOR('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 +140,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 +153,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:
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@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
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@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
participants (2)
-
Takashi Iwai -
Takashi Sakamoto