Re: [PATCH 2/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_process()

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Friday, August 18, 2023 5:58:46 PM CEST Peter Maydell wrote:
> Avoid a dynamic stack allocation in qjack_process().  Since this
> function is a JACK process callback, we are not permitted to malloc()
> here, so we allocate a working buffer in qjack_client_init() instead.
> 
> The codebase has very few VLAs, and if we can get rid of them all we
> can make the compiler error on new additions.  This is a defensive
> measure against security bugs where an on-stack dynamic allocation
> isn't correctly size-checked (e.g.  CVE-2021-3527).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This feels like we ought to be able to say "we know there are at most
> X channels, so allocate an array of size X on the stack", but I
> couldn't find anything in the audio subsystem from a quick look that
> set an obvious bound on the number of channels.  Is there some
> straightforward constant MAX_CHANNELS somewhere?
> ---

The JACK API doesn't have an official limit on "ports", but AFAICS in QEMU
there is a limit of max. 16 audio channels for audio frontends.

The QEMU `channels` CL option is apparently not limited ATM, but it might make
sense to limit that option to 16 channels as well. I mean anything beyond 16th
channel was a dead channel anyway, right?

Probably a separate battle field though:

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

>  audio/jackaudio.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 7cb2a49f971..e1eaa3477dc 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -70,6 +70,9 @@ typedef struct QJackClient {
>      int             buffersize;
>      jack_port_t   **port;
>      QJackBuffer     fifo;
> +
> +    /* Used as workspace by qjack_process() */
> +    float **process_buffers;
>  }
>  QJackClient;
>  
> @@ -267,22 +270,21 @@ static int qjack_process(jack_nframes_t nframes, void *arg)
>      }
>  
>      /* get the buffers for the ports */
> -    float *buffers[c->nchannels];
>      for (int i = 0; i < c->nchannels; ++i) {
> -        buffers[i] = jack_port_get_buffer(c->port[i], nframes);
> +        c->process_buffers[i] = jack_port_get_buffer(c->port[i], nframes);
>      }
>  
>      if (c->out) {
>          if (likely(c->enabled)) {
> -            qjack_buffer_read_l(&c->fifo, buffers, nframes);
> +            qjack_buffer_read_l(&c->fifo, c->process_buffers, nframes);
>          } else {
>              for (int i = 0; i < c->nchannels; ++i) {
> -                memset(buffers[i], 0, nframes * sizeof(float));
> +                memset(c->process_buffers[i], 0, nframes * sizeof(float));
>              }
>          }
>      } else {
>          if (likely(c->enabled)) {
> -            qjack_buffer_write_l(&c->fifo, buffers, nframes);
> +            qjack_buffer_write_l(&c->fifo, c->process_buffers, nframes);
>          }
>      }
>  
> @@ -448,6 +450,9 @@ static int qjack_client_init(QJackClient *c)
>            jack_get_client_name(c->client));
>      }
>  
> +    /* Allocate working buffer for process callback */
> +    c->process_buffers = g_new(float *, c->nchannels);
> +
>      jack_set_process_callback(c->client, qjack_process , c);
>      jack_set_port_registration_callback(c->client, qjack_port_registration, c);
>      jack_set_xrun_callback(c->client, qjack_xrun, c);
> @@ -579,6 +584,7 @@ static void qjack_client_fini_locked(QJackClient *c)
>  
>          qjack_buffer_free(&c->fifo);
>          g_free(c->port);
> +        g_free(c->process_buffers);
>  
>          c->state = QJACK_STATE_DISCONNECTED;
>          /* fallthrough */
>