[bug report] ALSA: xen-front: Use Xen common shared buffer implementation

[bug report] ALSA: xen-front: Use Xen common shared buffer implementation

[Dan Carpenter]

Hello Oleksandr Andrushchenko,

The patch 58f9d806d16a: "ALSA: xen-front: Use Xen common shared
buffer implementation" from Nov 30, 2018, leads to the following
static checker warning:

    sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
    warn: 'stream->shbuf.directory' double freed
    sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
    warn: 'stream->shbuf.grefs' double freed

sound/xen/xen_snd_front_alsa.c
   461  static int alsa_hw_params(struct snd_pcm_substream *substream,
   462                            struct snd_pcm_hw_params *params)
   463  {
   464          struct xen_snd_front_pcm_stream_info *stream = stream_get(substream);
   465          struct xen_snd_front_info *front_info = stream->front_info;
   466          struct xen_front_pgdir_shbuf_cfg buf_cfg;
   467          int ret;
   468  
   469          /*
   470           * This callback may be called multiple times,
   471           * so free the previously allocated shared buffer if any.
   472           */
   473          stream_free(stream);
                ^^^^^^^^^^^^^^^^^^^
This is freed here.

   474          ret = shbuf_setup_backstore(stream, params_buffer_bytes(params));
   475          if (ret < 0)
   476                  goto fail;
                        ^^^^^^^^^^
This leads to some double frees.  Probably more double frees than Smatch
is detecting.

   477  
   478          memset(&buf_cfg, 0, sizeof(buf_cfg));
   479          buf_cfg.xb_dev = front_info->xb_dev;
   480          buf_cfg.pgdir = &stream->shbuf;
   481          buf_cfg.num_pages = stream->num_pages;
   482          buf_cfg.pages = stream->pages;
   483  
   484          ret = xen_front_pgdir_shbuf_alloc(&buf_cfg);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is where "stream->shbuf.directory" is re-allocated on the success
path.

   485          if (ret < 0)
   486                  goto fail;
   487  
   488          ret = xen_front_pgdir_shbuf_map(&stream->shbuf);
   489          if (ret < 0)
   490                  goto fail;
   491  
   492          return 0;
   493  
   494  fail:
   495          stream_free(stream);
                ^^^^^^^^^^^^^^^^^^^^
Double free.

   496          dev_err(&front_info->xb_dev->dev,
   497                  "Failed to allocate buffers for stream with index %d\n",
   498                  stream->index);
   499          return ret;
   500  }

regards,
dan carpenter

Re: [bug report] ALSA: xen-front: Use Xen common shared buffer implementation

[Oleksandr Andrushchenko]

Hello, Dan!

On 10/21/20 1:50 PM, Dan Carpenter wrote:
> Hello Oleksandr Andrushchenko,
>
> The patch 58f9d806d16a: "ALSA: xen-front: Use Xen common shared
> buffer implementation" from Nov 30, 2018, leads to the following
> static checker warning:
>
>      sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
>      warn: 'stream->shbuf.directory' double freed
>      sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
>      warn: 'stream->shbuf.grefs' double freed
>
> sound/xen/xen_snd_front_alsa.c
>     461  static int alsa_hw_params(struct snd_pcm_substream *substream,
>     462                            struct snd_pcm_hw_params *params)
>     463  {
>     464          struct xen_snd_front_pcm_stream_info *stream = stream_get(substream);
>     465          struct xen_snd_front_info *front_info = stream->front_info;
>     466          struct xen_front_pgdir_shbuf_cfg buf_cfg;
>     467          int ret;
>     468
>     469          /*
>     470           * This callback may be called multiple times,
>     471           * so free the previously allocated shared buffer if any.
>     472           */
>     473          stream_free(stream);
>                  ^^^^^^^^^^^^^^^^^^^
> This is freed here.
>
>     474          ret = shbuf_setup_backstore(stream, params_buffer_bytes(params));
>     475          if (ret < 0)
>     476                  goto fail;
>                          ^^^^^^^^^^
> This leads to some double frees.  Probably more double frees than Smatch
> is detecting.
>
>     477
>     478          memset(&buf_cfg, 0, sizeof(buf_cfg));
>     479          buf_cfg.xb_dev = front_info->xb_dev;
>     480          buf_cfg.pgdir = &stream->shbuf;
>     481          buf_cfg.num_pages = stream->num_pages;
>     482          buf_cfg.pages = stream->pages;
>     483
>     484          ret = xen_front_pgdir_shbuf_alloc(&buf_cfg);
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is where "stream->shbuf.directory" is re-allocated on the success
> path.
>
>     485          if (ret < 0)
>     486                  goto fail;
>     487
>     488          ret = xen_front_pgdir_shbuf_map(&stream->shbuf);
>     489          if (ret < 0)
>     490                  goto fail;
>     491
>     492          return 0;
>     493
>     494  fail:
>     495          stream_free(stream);
>                  ^^^^^^^^^^^^^^^^^^^^
> Double free.
>
>     496          dev_err(&front_info->xb_dev->dev,
>     497                  "Failed to allocate buffers for stream with index %d\n",
>     498                  stream->index);
>     499          return ret;
>     500  }
>
> regards,
> dan carpenter

Thank you for reporting this,

I'll try to look at it closely and prepare a patch.

Thank you,

Oleksandr