[bug report] ALSA: rawmidi: Avoid racy info ioctl via ctl device

[Takashi Iwai]

On Mon, 01 Feb 2021 13:55:16 +0100,
Dan Carpenter wrote:
> 
> Hello Takashi Iwai,
> 
> The patch c1cfd9025cc3: "ALSA: rawmidi: Avoid racy info ioctl via ctl
> device" from Dec 14, 2017, leads to the following static checker
> warning:
> 
> 	sound/core/rawmidi.c:651 snd_rawmidi_info_select()
> 	warn: called with lock held.  '&register_mutex'
> 
> sound/core/seq/seq_midi.c
>    297          mutex_lock(&register_mutex);
>                            ^^^^^^^^^^^^^^^
> Holding this lock.
> 
>    298          client = synths[card->number];
>    299          if (client == NULL) {
>    300                  newclient = 1;
>    301                  client = kzalloc(sizeof(*client), GFP_KERNEL);
>    302                  if (client == NULL) {
>    303                          mutex_unlock(&register_mutex);
>    304                          kfree(info);
>    305                          return -ENOMEM;
>    306                  }
>    307                  client->seq_client =
>    308                          snd_seq_create_kernel_client(
>    309                                  card, 0, "%s", card->shortname[0] ?
>    310                                  (const char *)card->shortname : "External MIDI");
>    311                  if (client->seq_client < 0) {
>    312                          kfree(client);
>    313                          mutex_unlock(&register_mutex);
>    314                          kfree(info);
>    315                          return -ENOMEM;
>    316                  }
>    317          }
>    318  
>    319          msynth = kcalloc(ports, sizeof(struct seq_midisynth), GFP_KERNEL);
>    320          port = kmalloc(sizeof(*port), GFP_KERNEL);
>    321          if (msynth == NULL || port == NULL)
>    322                  goto __nomem;
>    323  
>    324          for (p = 0; p < ports; p++) {
>    325                  ms = &msynth[p];
>    326  
>    327                  if (snd_seq_midisynth_new(ms, card, device, p) < 0)
>    328                          goto __nomem;
>    329  
>    330                  /* declare port */
>    331                  memset(port, 0, sizeof(*port));
>    332                  port->addr.client = client->seq_client;
>    333                  port->addr.port = device * (256 / SNDRV_RAWMIDI_DEVICES) + p;
>    334                  port->flags = SNDRV_SEQ_PORT_FLG_GIVEN_PORT;
>    335                  memset(info, 0, sizeof(*info));
>    336                  info->device = device;
>    337                  if (p < output_count)
>    338                          info->stream = SNDRV_RAWMIDI_STREAM_OUTPUT;
>    339                  else
>    340                          info->stream = SNDRV_RAWMIDI_STREAM_INPUT;
>    341                  info->subdevice = p;
>    342                  if (snd_rawmidi_info_select(card, info) >= 0)
>                             ^^^^^^^^^^^^^^^^^^^^^^^
> We can't call this function when we're holding the lock or it leads to
> a deadlock.

Those two register_mutex are different ones; i.e. both are local static
variables, hence its scope is only for rawmidi.c and seq_client.c,
hence they can't conflict with each other.

Or am I missing something else?


thanks,

Takashi


> Before the patch, we used to rely on the callers to take
> the lock before calling snd_rawmidi_info_select() but the patch moved
> the lock inside the function.
> 
>    343                          strcpy(port->name, info->subname);
>    344                  if (! port->name[0]) {
>    345                          if (info->name[0]) {
>    346                                  if (ports > 1)
>    347                                          snprintf(port->name, sizeof(port->name), "%s-%u", info->name, p);
>    348                                  else
>    349                                          snprintf(port->name, sizeof(port->name), "%s", info->name);
> 
> regards,
> dan carpenter
>