[bug report] ALSA: seq: obsolete change of address limit

[bug report] ALSA: seq: obsolete change of address limit

[Dan Carpenter]

Hello Takashi Sakamoto,

The patch e12ec251e4db: "ALSA: seq: obsolete change of address limit"
from Aug 13, 2016, leads to the following static checker warning:

	sound/core/seq/seq_compat.c:61 snd_seq_call_port_info_ioctl()
	warn: did you mean to pass the address of 'data'

sound/core/seq/seq_compat.c
    45  static int snd_seq_call_port_info_ioctl(struct snd_seq_client *client, unsigned int cmd,
    46                                          struct snd_seq_port_info32 __user *data32)
    47  {
    48          int err = -EFAULT;
    49          struct snd_seq_port_info *data;
    50  
    51          data = kmalloc(sizeof(*data), GFP_KERNEL);
    52          if (!data)
    53                  return -ENOMEM;
    54  
    55          if (copy_from_user(data, data32, sizeof(*data32)) ||
    56              get_user(data->flags, &data32->flags) ||
    57              get_user(data->time_queue, &data32->time_queue))
    58                  goto error;
    59          data->kernel = NULL;
    60  
    61          err = snd_seq_kernel_client_ctl(client->number, cmd, &data);

This should almost certainly be "cmd, data);" without the &.  Have you
tested this?  It eventually gets passed to functions like
snd_seq_ioctl_create_port().

    62          if (err < 0)
    63                  goto error;
    64  
    65          if (copy_to_user(data32, data, sizeof(*data32)) ||
    66              put_user(data->flags, &data32->flags) ||
    67              put_user(data->time_queue, &data32->time_queue))
    68                  err = -EFAULT;
    69  
    70   error:
    71          kfree(data);
    72          return err;
    73  }

regards,
dan carpenter

Re: [bug report] ALSA: seq: obsolete change of address limit

[Takashi Sakamoto]

Hi Dan,

On Oct 11 2016 20:06, Dan Carpenter wrote:
> Hello Takashi Sakamoto,
> 
> The patch e12ec251e4db: "ALSA: seq: obsolete change of address limit"
> from Aug 13, 2016, leads to the following static checker warning:
> 
> 	sound/core/seq/seq_compat.c:61 snd_seq_call_port_info_ioctl()
> 	warn: did you mean to pass the address of 'data'
> 
> sound/core/seq/seq_compat.c
>     45  static int snd_seq_call_port_info_ioctl(struct snd_seq_client *client, unsigned int cmd,
>     46                                          struct snd_seq_port_info32 __user *data32)
>     47  {
>     48          int err = -EFAULT;
>     49          struct snd_seq_port_info *data;
>     50  
>     51          data = kmalloc(sizeof(*data), GFP_KERNEL);
>     52          if (!data)
>     53                  return -ENOMEM;
>     54  
>     55          if (copy_from_user(data, data32, sizeof(*data32)) ||
>     56              get_user(data->flags, &data32->flags) ||
>     57              get_user(data->time_queue, &data32->time_queue))
>     58                  goto error;
>     59          data->kernel = NULL;
>     60  
>     61          err = snd_seq_kernel_client_ctl(client->number, cmd, &data);
> 
> This should almost certainly be "cmd, data);" without the &.  Have you
> tested this?  It eventually gets passed to functions like
> snd_seq_ioctl_create_port().

Indeed. This is a bug. I might not have tested compatibility layer...

Just now I posted a fix for this bug. With this patch, it passes this
simple ioctl test.
https://github.com/takaswie/alsa-ioctl-test


Thanks!

Takashi Sakamoto