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

1 Feb 2021


      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