[PATCH 0/2] audio/jackaudio: avoid dynamic stack allocations

[PATCH 0/2] audio/jackaudio: avoid dynamic stack allocations

[Peter Maydell]

This patchset removes two variable length arrays from the jack audio
backend.  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).

The first one is fairly straightforward (although the JACK API's
requirement that (a) you don't pass it an overlong client name and
(b) that maximum length is provided by calling a function, not as a
compile time constant makes it a little less clean than it might be.

The second one avoids the dynamic allocation, but if the audio
subsystem has a compile-time upper bound on the number of
channels then we could use a fixed-size stack array rather than
the awkward "allocate a working buffer at init time" that I
have in this patch. Suggestions for improvements welcome.

Disclaimer: tested only with "make check", which doesn't actually
exercise the audio subsystem.

thanks
-- PMM

Peter Maydell (2):
  audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init
  audio/jackaudio: Avoid dynamic stack allocation in qjack_process()

 audio/jackaudio.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init

[Peter Maydell]

Avoid a dynamic stack allocation in qjack_client_init(), by using
a g_autofree heap allocation instead.

(We stick with allocate + snprintf() because the JACK API requires
the name to be no more than its maximum size, so g_strdup_printf()
would require an extra truncation step.)

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>
---
 audio/jackaudio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 5bdf3d7a78d..7cb2a49f971 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -400,7 +400,8 @@ static void qjack_client_connect_ports(QJackClient *c)
 static int qjack_client_init(QJackClient *c)
 {
     jack_status_t status;
-    char client_name[jack_client_name_size()];
+    int client_name_len = jack_client_name_size(); /* includes NUL */
+    g_autofree char *client_name = g_new(char, client_name_len);
     jack_options_t options = JackNullOption;
 
     if (c->state == QJACK_STATE_RUNNING) {
@@ -409,7 +410,7 @@ static int qjack_client_init(QJackClient *c)
 
     c->connect_ports = true;
 
-    snprintf(client_name, sizeof(client_name), "%s-%s",
+    snprintf(client_name, client_name_len, "%s-%s",
         c->out ? "out" : "in",
         c->opt->client_name ? c->opt->client_name : audio_application_name());
 
-- 
2.34.1

Re: [PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init

[Francisco Iglesias]

On [2023 Aug 18] Fri 16:58:45, Peter Maydell wrote:
> Avoid a dynamic stack allocation in qjack_client_init(), by using
> a g_autofree heap allocation instead.
> 
> (We stick with allocate + snprintf() because the JACK API requires
> the name to be no more than its maximum size, so g_strdup_printf()
> would require an extra truncation step.)
> 
> 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>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  audio/jackaudio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 5bdf3d7a78d..7cb2a49f971 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -400,7 +400,8 @@ static void qjack_client_connect_ports(QJackClient *c)
>  static int qjack_client_init(QJackClient *c)
>  {
>      jack_status_t status;
> -    char client_name[jack_client_name_size()];
> +    int client_name_len = jack_client_name_size(); /* includes NUL */
> +    g_autofree char *client_name = g_new(char, client_name_len);
>      jack_options_t options = JackNullOption;
>  
>      if (c->state == QJACK_STATE_RUNNING) {
> @@ -409,7 +410,7 @@ static int qjack_client_init(QJackClient *c)
>  
>      c->connect_ports = true;
>  
> -    snprintf(client_name, sizeof(client_name), "%s-%s",
> +    snprintf(client_name, client_name_len, "%s-%s",
>          c->out ? "out" : "in",
>          c->opt->client_name ? c->opt->client_name : audio_application_name());
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init

[Christian Schoenebeck]

On Friday, August 18, 2023 5:58:45 PM CEST Peter Maydell wrote:
> Avoid a dynamic stack allocation in qjack_client_init(), by using
> a g_autofree heap allocation instead.
> 
> (We stick with allocate + snprintf() because the JACK API requires
> the name to be no more than its maximum size, so g_strdup_printf()
> would require an extra truncation step.)
> 
> 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).

Sounds good, what compiler flag will that be?

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  audio/jackaudio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 5bdf3d7a78d..7cb2a49f971 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -400,7 +400,8 @@ static void qjack_client_connect_ports(QJackClient *c)
>  static int qjack_client_init(QJackClient *c)
>  {
>      jack_status_t status;
> -    char client_name[jack_client_name_size()];
> +    int client_name_len = jack_client_name_size(); /* includes NUL */

I would add `const` here.

> +    g_autofree char *client_name = g_new(char, client_name_len);
>      jack_options_t options = JackNullOption;
>  
>      if (c->state == QJACK_STATE_RUNNING) {
> @@ -409,7 +410,7 @@ static int qjack_client_init(QJackClient *c)
>  
>      c->connect_ports = true;
>  
> -    snprintf(client_name, sizeof(client_name), "%s-%s",
> +    snprintf(client_name, client_name_len, "%s-%s",
>          c->out ? "out" : "in",
>          c->opt->client_name ? c->opt->client_name : audio_application_name());

Unrelated, but this could be shortened by Elvis operator BTW:

    c->opt->client_name ?: audio_application_name()

Anyway:

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

Best regards,
Christian Schoenebeck

Re: [PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init

[Peter Maydell]

On Mon, 21 Aug 2023 at 09:01, Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> On Friday, August 18, 2023 5:58:45 PM CEST Peter Maydell wrote:
> > Avoid a dynamic stack allocation in qjack_client_init(), by using
> > a g_autofree heap allocation instead.
> >
> > (We stick with allocate + snprintf() because the JACK API requires
> > the name to be no more than its maximum size, so g_strdup_printf()
> > would require an extra truncation step.)
> >
> > 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).
>
> Sounds good, what compiler flag will that be?

It's "-Wvla".

> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  audio/jackaudio.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> > index 5bdf3d7a78d..7cb2a49f971 100644
> > --- a/audio/jackaudio.c
> > +++ b/audio/jackaudio.c
> > @@ -400,7 +400,8 @@ static void qjack_client_connect_ports(QJackClient *c)
> >  static int qjack_client_init(QJackClient *c)
> >  {
> >      jack_status_t status;
> > -    char client_name[jack_client_name_size()];
> > +    int client_name_len = jack_client_name_size(); /* includes NUL */
>
> I would add `const` here.

Sure, I can do this. (I tend not to use 'const' except when
dealing with pointers, personally, but that's just habit.)

> > +    g_autofree char *client_name = g_new(char, client_name_len);
> >      jack_options_t options = JackNullOption;
> >
> >      if (c->state == QJACK_STATE_RUNNING) {
> > @@ -409,7 +410,7 @@ static int qjack_client_init(QJackClient *c)
> >
> >      c->connect_ports = true;
> >
> > -    snprintf(client_name, sizeof(client_name), "%s-%s",
> > +    snprintf(client_name, client_name_len, "%s-%s",
> >          c->out ? "out" : "in",
> >          c->opt->client_name ? c->opt->client_name : audio_application_name());
>
> Unrelated, but this could be shortened by Elvis operator BTW:
>
>     c->opt->client_name ?: audio_application_name()
>
> Anyway:
>
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

thanks
-- PMM

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

[Peter Maydell]

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?
---
 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 */
-- 
2.34.1

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

[Francisco Iglesias]

On [2023 Aug 18] Fri 16:58:46, 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>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
> 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?
> ---
>  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 */
> -- 
> 2.34.1
> 
> 

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

[Christian Schoenebeck]

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 */
> 

Re: [PATCH 0/2] audio/jackaudio: avoid dynamic stack allocations

[Marc-André Lureau]

On Fri, Aug 18, 2023 at 7:59 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset removes two variable length arrays from the jack audio
> backend.  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).
>
> The first one is fairly straightforward (although the JACK API's
> requirement that (a) you don't pass it an overlong client name and
> (b) that maximum length is provided by calling a function, not as a
> compile time constant makes it a little less clean than it might be.
>
> The second one avoids the dynamic allocation, but if the audio
> subsystem has a compile-time upper bound on the number of
> channels then we could use a fixed-size stack array rather than
> the awkward "allocate a working buffer at init time" that I
> have in this patch. Suggestions for improvements welcome.
>
> Disclaimer: tested only with "make check", which doesn't actually
> exercise the audio subsystem.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


-- 
Marc-André Lureau

Re: [PATCH 0/2] audio/jackaudio: avoid dynamic stack allocations

[Peter Maydell]

Hi Gerd; this series has been reviewed. Did you want to take it
via the audio tree? Or I can put it in via target-arm.next
if you prefer.

thanks
-- PMM

On Fri, 18 Aug 2023 at 16:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset removes two variable length arrays from the jack audio
> backend.  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).
>
> The first one is fairly straightforward (although the JACK API's
> requirement that (a) you don't pass it an overlong client name and
> (b) that maximum length is provided by calling a function, not as a
> compile time constant makes it a little less clean than it might be.
>
> The second one avoids the dynamic allocation, but if the audio
> subsystem has a compile-time upper bound on the number of
> channels then we could use a fixed-size stack array rather than
> the awkward "allocate a working buffer at init time" that I
> have in this patch. Suggestions for improvements welcome.
>
> Disclaimer: tested only with "make check", which doesn't actually
> exercise the audio subsystem.
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init
>   audio/jackaudio: Avoid dynamic stack allocation in qjack_process()
>
>  audio/jackaudio.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)

Re: [PATCH 0/2] audio/jackaudio: avoid dynamic stack allocations

[Gerd Hoffmann]

On Tue, Sep 12, 2023 at 03:19:08PM +0100, Peter Maydell wrote:
> Hi Gerd; this series has been reviewed. Did you want to take it
> via the audio tree? Or I can put it in via target-arm.next
> if you prefer.

arm tree is fine with me.

take care,
  Gerd