Re: [PATCH 2/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_process()
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
> ---
> 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
> 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(>fifo, buffers, nframes);
> +qjack_buffer_read_l(>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(>fifo, buffers, nframes);
> +qjack_buffer_write_l(>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(>fifo);
> g_free(c->port);
> +g_free(c->process_buffers);
>
> c->state = QJACK_STATE_DISCONNECTED;
> /* fallthrough */
>
Re: [PATCH 2/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_process()
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
Reviewed-by: Francisco Iglesias
> ---
> 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(>fifo, buffers, nframes);
> +qjack_buffer_read_l(>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(>fifo, buffers, nframes);
> +qjack_buffer_write_l(>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(>fifo);
> g_free(c->port);
> +g_free(c->process_buffers);
>
> c->state = QJACK_STATE_DISCONNECTED;
> /* fallthrough */
> --
> 2.34.1
>
>
[PATCH 2/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_process()
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
---
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(>fifo, buffers, nframes);
+qjack_buffer_read_l(>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(>fifo, buffers, nframes);
+qjack_buffer_write_l(>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(>fifo);
g_free(c->port);
+g_free(c->process_buffers);
c->state = QJACK_STATE_DISCONNECTED;
/* fallthrough */
--
2.34.1
