[-,maemo,fixes,1/1] maemo plugin has two crashes I was able to see in a valgrind log from another user:

Commit Message

[e-mail] May 2, 2019, 9:26 p.m. UTC

From: Stian Skjelstad <[e-mail]>

* maximum write size was calculated in words (16bit), but checked against
  byte-size length. This causes memcpy later to overflow the buffer
  (normally by up to 12KB).
* remove a double free (by marking free'd data with NULL)

* mmap returns MMAP_FAILED on error, not NULL

I suspect that this plugin/driver might have other issues aswell, since I
am unable to find any logic for checking DSP buffer status, and no
implementation for odelay reporting.

Author: Stian Skjelstad <[e-mail]>
Signed-off-by: Stian Skjelstad <[e-mail]>

Comments

Takashi Iwai May 5, 2019, 7:45 a.m. UTC | #1

On Thu, 02 May 2019 23:26:15 +0200,
[e-mail] wrote:
> 
> From: Stian Skjelstad <[e-mail]>
> 
> * maximum write size was calculated in words (16bit), but checked against
>   byte-size length. This causes memcpy later to overflow the buffer
>   (normally by up to 12KB).
> * remove a double free (by marking free'd data with NULL)
> 
> * mmap returns MMAP_FAILED on error, not NULL
> 
> I suspect that this plugin/driver might have other issues aswell, since I
> am unable to find any logic for checking DSP buffer status, and no
> implementation for odelay reporting.
> 
> Author: Stian Skjelstad <[e-mail]>
> Signed-off-by: Stian Skjelstad <[e-mail]>

Thanks, applied now.


Takashi

Patch

diff --git a/maemo/alsa-dsp.c b/maemo/alsa-dsp.c
index 7e04f6a..3307384 100644
--- a/maemo/alsa-dsp.c
+++ b/maemo/alsa-dsp.c
@@ -135,18 +135,18 @@  static snd_pcm_sframes_t alsa_dsp_transfer(snd_pcm_ioplug_t * io,
 	snd_pcm_alsa_dsp_t *alsa_dsp = io->private_data;
 	DENTER();
 	char *buf;
-	int words;
+	int bytes, words;
 	ssize_t result;
 
-	words = size * alsa_dsp->bytes_per_frame;
-	words /= 2;
-	DPRINT("***** Info: words %d size %lu bpf: %d\n", words, size,
-	       alsa_dsp->bytes_per_frame);
-	if (words > alsa_dsp->dsp_protocol->mmap_buffer_size) {
-		DERROR("Requested too much data transfer (playing only %d)\n",
-		       alsa_dsp->dsp_protocol->mmap_buffer_size);
-		words = alsa_dsp->dsp_protocol->mmap_buffer_size;
+	bytes = size * alsa_dsp->bytes_per_frame;
+	DPRINT("***** Info: samples %lu * bpf %d => bytes %d\n",
+	       size, alsa_dsp->bytes_per_frame, bytes);
+	if (bytes > alsa_dsp->dsp_protocol->mmap_buffer_size) {
+		DERROR("Requested too much data transfer (requested %d, playing only %d)\n",
+		       bytes, alsa_dsp->dsp_protocol->mmap_buffer_size);
+		bytes = alsa_dsp->dsp_protocol->mmap_buffer_size;
 	}
+	words = bytes / 2;
 	if (alsa_dsp->dsp_protocol->state != STATE_PLAYING) {
 		DPRINT("I did nothing - No start sent\n");
 		alsa_dsp_start(io);
diff --git a/maemo/dsp-ctl.c b/maemo/dsp-ctl.c
index 5bcda36..ac05942 100644
--- a/maemo/dsp-ctl.c
+++ b/maemo/dsp-ctl.c
@@ -93,6 +93,7 @@  static void dsp_ctl_close(snd_ctl_ext_t * ext)
 	snd_ctl_dsp_t *dsp_ctl = ext->private_data;
 	DENTER();
 	free(dsp_ctl->controls);
+	dsp_ctl->controls = NULL;
 	free_control_list(&(dsp_ctl->playback_devices));
 	free_control_list(&(dsp_ctl->recording_devices));
 //      free(dsp_ctl);
diff --git a/maemo/dsp-protocol.c b/maemo/dsp-protocol.c
index af67251..32193d3 100644
--- a/maemo/dsp-protocol.c
+++ b/maemo/dsp-protocol.c
@@ -194,7 +194,7 @@  int dsp_protocol_open_node(dsp_protocol_t * dsp_protocol, const char *device)
 	    mmap((void *)0, dsp_protocol->mmap_buffer_size,
 		 PROT_READ | PROT_WRITE, MAP_SHARED, dsp_protocol->fd, 0);
 
-	if (dsp_protocol->mmap_buffer == NULL) {
+	if (dsp_protocol->mmap_buffer == MAP_FAILED) {
 		report_dsp_protocol("Cannot mmap data buffer", dsp_protocol);
 		ret = -ENOMEM;
 		goto unlock;