Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Takashi Iwai]

On Mon, 26 Jun 2023 05:42:29 +0200,
Tuo Li wrote:
> 
> 
> Hello,
> 
> Our static analysis tool finds a possible data race in ALSA in Linux 6.4.0.
> 
> In some functions, the field snd_card.total_pcm_alloc_bytes is accessed
> with holding the lock snd_card.memory_mutex. Here is an example:
> 
>   do_free_pages() --> Line 57
>     mutex_lock(&card->memory_mutex); --> Line 61 (Lock card->memory_mutex)
>     card->total_pcm_alloc_bytes -= dmab->bytes;  --> Line 63 (Access  card->
> total_pcm_alloc_bytes)
> 
> However, in the function do_alloc_pages():
> 
>   if (max_alloc_per_card &&
>     card->total_pcm_alloc_bytes + size > max_alloc_per_card) --> Line 41
> 
> the variable card->total_pcm_alloc_bytes is accessed without holding
> the lock card->memory_mutex, and thus a data race can occur.
> 
> In my opinion, this data race may be harmful, because the value of
> card->total_pcm_alloc_bytes may be changed by another thread after
> the if check. Therefore, its value may be too large after Line 51 and can
> cause memory bugs such as buffer overflow:
> 
>   card->total_pcm_alloc_bytes += dmab->bytes;  --> Line 51
> 
> I am not quite sure whether this possible data race is real and how to
> fix it if it is real.
> 
> Any feedback would be appreciated, thanks!
> 
> Reported-by: BassCheck <bass@buaa.edu.cn>

It's a bit racy indeed, but the effect is almost negligible.  The size
check there is merely a sanity check, and allocating more bytes
doesn't mean to conflict against anything practically.

That said, it's a better-to-be-addressed bug, but nothing too
serious.


thanks,

Takashi

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Takashi Iwai]

On Mon, 26 Jun 2023 09:31:18 +0200,
Tuo Li wrote:
> 
> 
> Hello,
> 
> Thank you for your reply!

FWIW, the simplest fix would be something like below, just extending
the mutex coverage.  But it'll serialize the all calls, so it might
influence on the performance, while it's the safest way.


Takashi

--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -37,20 +37,22 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
 	enum dma_data_direction dir;
 	int err;
 
+	mutex_lock(&card->memory_mutex);
 	if (max_alloc_per_card &&
-	    card->total_pcm_alloc_bytes + size > max_alloc_per_card)
-		return -ENOMEM;
+	    card->total_pcm_alloc_bytes + size > max_alloc_per_card) {
+		err = -ENOMEM;
+		goto unlock;
+	}
 
 	if (str == SNDRV_PCM_STREAM_PLAYBACK)
 		dir = DMA_TO_DEVICE;
 	else
 		dir = DMA_FROM_DEVICE;
 	err = snd_dma_alloc_dir_pages(type, dev, dir, size, dmab);
-	if (!err) {
-		mutex_lock(&card->memory_mutex);
+	if (!err)
 		card->total_pcm_alloc_bytes += dmab->bytes;
-		mutex_unlock(&card->memory_mutex);
-	}
+ unlock:
+	mutex_unlock(&card->memory_mutex);
 	return err;
 }
 

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Jaroslav Kysela]

On 26. 06. 23 9:33, Takashi Iwai wrote:
> On Mon, 26 Jun 2023 09:31:18 +0200,
> Tuo Li wrote:
>>
>>
>> Hello,
>>
>> Thank you for your reply!
> 
> FWIW, the simplest fix would be something like below, just extending
> the mutex coverage.  But it'll serialize the all calls, so it might
> influence on the performance, while it's the safest way.

It may be better to update total_pcm_alloc_bytes before 
snd_dma_alloc_dir_pages() call and decrease this value when allocation fails 
to allow parallel allocations. Then the mutex can be held only for the 
total_pcm_alloc_bytes variable update.

Eventually, total_pcm_alloc_bytes may be atomic.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Takashi Iwai]

On Mon, 26 Jun 2023 09:56:47 +0200,
Jaroslav Kysela wrote:
> 
> On 26. 06. 23 9:33, Takashi Iwai wrote:
> > On Mon, 26 Jun 2023 09:31:18 +0200,
> > Tuo Li wrote:
> >> 
> >> 
> >> Hello,
> >> 
> >> Thank you for your reply!
> > 
> > FWIW, the simplest fix would be something like below, just extending
> > the mutex coverage.  But it'll serialize the all calls, so it might
> > influence on the performance, while it's the safest way.
> 
> It may be better to update total_pcm_alloc_bytes before
> snd_dma_alloc_dir_pages() call and decrease this value when allocation
> fails to allow parallel allocations. Then the mutex can be held only
> for the total_pcm_alloc_bytes variable update.

Yes, it'd work.  But a tricky part is that the actual allocation size
can be bigger, and we need to correct the total_pcm_alloc_bytes after
the allocation result.  So the end result would be a patch like below,
which is a bit more complex than the previous simpler approach.  But
it might be OK.

> Eventually, total_pcm_alloc_bytes may be atomic.

Possible, but the same problem like the above applies, so I believe
the mutex is good enough.

Another alternative would be to move the size check after the
successful allocation, assuming that the size exceeds a very
exceptional scenario.  The code flow would be a bit simpler.


thanks,

Takashi

--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -37,9 +37,14 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
 	enum dma_data_direction dir;
 	int err;
 
+	mutex_lock(&card->memory_mutex);
 	if (max_alloc_per_card &&
-	    card->total_pcm_alloc_bytes + size > max_alloc_per_card)
+	    card->total_pcm_alloc_bytes + size > max_alloc_per_card) {
+		mutex_unlock(&card->memory_mutex);
 		return -ENOMEM;
+	}
+	card->total_pcm_alloc_bytes += size		
+	mutex_unlock(&card->memory_mutex);
 
 	if (str == SNDRV_PCM_STREAM_PLAYBACK)
 		dir = DMA_TO_DEVICE;
@@ -47,8 +52,18 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
 		dir = DMA_FROM_DEVICE;
 	err = snd_dma_alloc_dir_pages(type, dev, dir, size, dmab);
 	if (!err) {
+		/* the actual allocation size might be bigger than requested,
+		 * and we need to correct the account
+		 */
+		if (dmab->bytes != size) {
+			mutex_lock(&card->memory_mutex);
+			card->total_pcm_alloc_bytes += dmab->bytes - size;
+			mutex_unlock(&card->memory_mutex);
+		}
+	} else {
+		/* allocation failure, take back */
 		mutex_lock(&card->memory_mutex);
-		card->total_pcm_alloc_bytes += dmab->bytes;
+		card->total_pcm_alloc_bytes -= size;
 		mutex_unlock(&card->memory_mutex);
 	}
 	return err;

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Jaroslav Kysela]

On 26. 06. 23 13:02, Takashi Iwai wrote:
> On Mon, 26 Jun 2023 09:56:47 +0200,
> Jaroslav Kysela wrote:
>>
>> On 26. 06. 23 9:33, Takashi Iwai wrote:
>>> On Mon, 26 Jun 2023 09:31:18 +0200,
>>> Tuo Li wrote:
>>>>
>>>>
>>>> Hello,
>>>>
>>>> Thank you for your reply!
>>>
>>> FWIW, the simplest fix would be something like below, just extending
>>> the mutex coverage.  But it'll serialize the all calls, so it might
>>> influence on the performance, while it's the safest way.
>>
>> It may be better to update total_pcm_alloc_bytes before
>> snd_dma_alloc_dir_pages() call and decrease this value when allocation
>> fails to allow parallel allocations. Then the mutex can be held only
>> for the total_pcm_alloc_bytes variable update.
> 
> Yes, it'd work.  But a tricky part is that the actual allocation size
> can be bigger, and we need to correct the total_pcm_alloc_bytes after
> the allocation result.  So the end result would be a patch like below,
> which is a bit more complex than the previous simpler approach.  But
> it might be OK.

The patch looks good, but it may be better to move the "post" variable updates 
to an inline function (mutex lock - update - mutex unlock) for a better 
readability.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Takashi Iwai]

On Mon, 26 Jun 2023 13:09:00 +0200,
Jaroslav Kysela wrote:
> 
> On 26. 06. 23 13:02, Takashi Iwai wrote:
> > On Mon, 26 Jun 2023 09:56:47 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> On 26. 06. 23 9:33, Takashi Iwai wrote:
> >>> On Mon, 26 Jun 2023 09:31:18 +0200,
> >>> Tuo Li wrote:
> >>>> 
> >>>> 
> >>>> Hello,
> >>>> 
> >>>> Thank you for your reply!
> >>> 
> >>> FWIW, the simplest fix would be something like below, just extending
> >>> the mutex coverage.  But it'll serialize the all calls, so it might
> >>> influence on the performance, while it's the safest way.
> >> 
> >> It may be better to update total_pcm_alloc_bytes before
> >> snd_dma_alloc_dir_pages() call and decrease this value when allocation
> >> fails to allow parallel allocations. Then the mutex can be held only
> >> for the total_pcm_alloc_bytes variable update.
> > 
> > Yes, it'd work.  But a tricky part is that the actual allocation size
> > can be bigger, and we need to correct the total_pcm_alloc_bytes after
> > the allocation result.  So the end result would be a patch like below,
> > which is a bit more complex than the previous simpler approach.  But
> > it might be OK.
> 
> The patch looks good, but it may be better to move the "post" variable
> updates to an inline function (mutex lock - update - mutex unlock) for
> a better readability.

Sounds like a good idea.  Let me cook later.


Takashi

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Takashi Iwai]

On Mon, 26 Jun 2023 13:13:21 +0200,
Takashi Iwai wrote:
> 
> On Mon, 26 Jun 2023 13:09:00 +0200,
> Jaroslav Kysela wrote:
> > 
> > On 26. 06. 23 13:02, Takashi Iwai wrote:
> > > On Mon, 26 Jun 2023 09:56:47 +0200,
> > > Jaroslav Kysela wrote:
> > >> 
> > >> On 26. 06. 23 9:33, Takashi Iwai wrote:
> > >>> On Mon, 26 Jun 2023 09:31:18 +0200,
> > >>> Tuo Li wrote:
> > >>>> 
> > >>>> 
> > >>>> Hello,
> > >>>> 
> > >>>> Thank you for your reply!
> > >>> 
> > >>> FWIW, the simplest fix would be something like below, just extending
> > >>> the mutex coverage.  But it'll serialize the all calls, so it might
> > >>> influence on the performance, while it's the safest way.
> > >> 
> > >> It may be better to update total_pcm_alloc_bytes before
> > >> snd_dma_alloc_dir_pages() call and decrease this value when allocation
> > >> fails to allow parallel allocations. Then the mutex can be held only
> > >> for the total_pcm_alloc_bytes variable update.
> > > 
> > > Yes, it'd work.  But a tricky part is that the actual allocation size
> > > can be bigger, and we need to correct the total_pcm_alloc_bytes after
> > > the allocation result.  So the end result would be a patch like below,
> > > which is a bit more complex than the previous simpler approach.  But
> > > it might be OK.
> > 
> > The patch looks good, but it may be better to move the "post" variable
> > updates to an inline function (mutex lock - update - mutex unlock) for
> > a better readability.
> 
> Sounds like a good idea.  Let me cook later.

... and here it is.

If that looks OK, I'll submit a proper fix patch.


thanks,

Takashi

--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -31,15 +31,41 @@ static unsigned long max_alloc_per_card = 32UL * 1024UL * 1024UL;
 module_param(max_alloc_per_card, ulong, 0644);
 MODULE_PARM_DESC(max_alloc_per_card, "Max total allocation bytes per card.");
 
+static void __update_allocated_size(struct snd_card *card, ssize_t bytes)
+{
+	card->total_pcm_alloc_bytes += bytes;
+}
+
+static void update_allocated_size(struct snd_card *card, ssize_t bytes)
+{
+	mutex_lock(&card->memory_mutex);
+	__update_allocated_size(card, bytes);
+	mutex_unlock(&card->memory_mutex);
+}
+
+static void decrease_allocated_size(struct snd_card *card, size_t bytes)
+{
+	mutex_lock(&card->memory_mutex);
+	WARN_ON(card->total_pcm_alloc_bytes < bytes);
+	__update_allocated_size(card, -(ssize_t)bytes);
+	mutex_unlock(&card->memory_mutex);
+}
+
 static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
 			  int str, size_t size, struct snd_dma_buffer *dmab)
 {
 	enum dma_data_direction dir;
 	int err;
 
+	/* check and reserve the requested size */
+	mutex_lock(&card->memory_mutex);
 	if (max_alloc_per_card &&
-	    card->total_pcm_alloc_bytes + size > max_alloc_per_card)
+	    card->total_pcm_alloc_bytes + size > max_alloc_per_card) {
+		mutex_unlock(&card->memory_mutex);
 		return -ENOMEM;
+	}
+	__update_allocated_size(card, size);
+	mutex_unlock(&card->memory_mutex);
 
 	if (str == SNDRV_PCM_STREAM_PLAYBACK)
 		dir = DMA_TO_DEVICE;
@@ -47,9 +73,14 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
 		dir = DMA_FROM_DEVICE;
 	err = snd_dma_alloc_dir_pages(type, dev, dir, size, dmab);
 	if (!err) {
-		mutex_lock(&card->memory_mutex);
-		card->total_pcm_alloc_bytes += dmab->bytes;
-		mutex_unlock(&card->memory_mutex);
+		/* the actual allocation size might be bigger than requested,
+		 * and we need to correct the account
+		 */
+		if (dmab->bytes != size)
+			update_allocated_size(card, dmab->bytes - size);
+	} else {
+		/* take back on allocation failure */
+		decrease_allocated_size(card, size);
 	}
 	return err;
 }
@@ -58,10 +89,7 @@ static void do_free_pages(struct snd_card *card, struct snd_dma_buffer *dmab)
 {
 	if (!dmab->area)
 		return;
-	mutex_lock(&card->memory_mutex);
-	WARN_ON(card->total_pcm_alloc_bytes < dmab->bytes);
-	card->total_pcm_alloc_bytes -= dmab->bytes;
-	mutex_unlock(&card->memory_mutex);
+	decrease_allocated_size(card, dmab->bytes);
 	snd_dma_free_pages(dmab);
 	dmab->area = NULL;
 }

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Jaroslav Kysela]

On 26. 06. 23 15:15, Takashi Iwai wrote:
> On Mon, 26 Jun 2023 13:13:21 +0200,
> Takashi Iwai wrote:
>>
>> On Mon, 26 Jun 2023 13:09:00 +0200,
>> Jaroslav Kysela wrote:
>>>
>>> On 26. 06. 23 13:02, Takashi Iwai wrote:
>>>> On Mon, 26 Jun 2023 09:56:47 +0200,
>>>> Jaroslav Kysela wrote:
>>>>>
>>>>> On 26. 06. 23 9:33, Takashi Iwai wrote:
>>>>>> On Mon, 26 Jun 2023 09:31:18 +0200,
>>>>>> Tuo Li wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Thank you for your reply!
>>>>>>
>>>>>> FWIW, the simplest fix would be something like below, just extending
>>>>>> the mutex coverage.  But it'll serialize the all calls, so it might
>>>>>> influence on the performance, while it's the safest way.
>>>>>
>>>>> It may be better to update total_pcm_alloc_bytes before
>>>>> snd_dma_alloc_dir_pages() call and decrease this value when allocation
>>>>> fails to allow parallel allocations. Then the mutex can be held only
>>>>> for the total_pcm_alloc_bytes variable update.
>>>>
>>>> Yes, it'd work.  But a tricky part is that the actual allocation size
>>>> can be bigger, and we need to correct the total_pcm_alloc_bytes after
>>>> the allocation result.  So the end result would be a patch like below,
>>>> which is a bit more complex than the previous simpler approach.  But
>>>> it might be OK.
>>>
>>> The patch looks good, but it may be better to move the "post" variable
>>> updates to an inline function (mutex lock - update - mutex unlock) for
>>> a better readability.
>>
>> Sounds like a good idea.  Let me cook later.
> 
> ... and here it is.
> 
> If that looks OK, I'll submit a proper fix patch.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/core/pcm_memory.c
> +++ b/sound/core/pcm_memory.c
> @@ -31,15 +31,41 @@ static unsigned long max_alloc_per_card = 32UL * 1024UL * 1024UL;
>   module_param(max_alloc_per_card, ulong, 0644);
>   MODULE_PARM_DESC(max_alloc_per_card, "Max total allocation bytes per card.");
>   
> +static void __update_allocated_size(struct snd_card *card, ssize_t bytes)

Missing inline ? May be also used for

> +static void update_allocated_size(struct snd_card *card, ssize_t bytes)
> +static void decrease_allocated_size(struct snd_card *card, size_t bytes)

The rest is fine in my eyes.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

				Thanks,
					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [BUG] ALSA: core: pcm_memory: a possible data race in do_alloc_pages()

[Takashi Iwai]

On Mon, 26 Jun 2023 15:32:40 +0200,
Jaroslav Kysela wrote:
> 
> On 26. 06. 23 15:15, Takashi Iwai wrote:
> > On Mon, 26 Jun 2023 13:13:21 +0200,
> > Takashi Iwai wrote:
> >> 
> >> On Mon, 26 Jun 2023 13:09:00 +0200,
> >> Jaroslav Kysela wrote:
> >>> 
> >>> On 26. 06. 23 13:02, Takashi Iwai wrote:
> >>>> On Mon, 26 Jun 2023 09:56:47 +0200,
> >>>> Jaroslav Kysela wrote:
> >>>>> 
> >>>>> On 26. 06. 23 9:33, Takashi Iwai wrote:
> >>>>>> On Mon, 26 Jun 2023 09:31:18 +0200,
> >>>>>> Tuo Li wrote:
> >>>>>>> 
> >>>>>>> 
> >>>>>>> Hello,
> >>>>>>> 
> >>>>>>> Thank you for your reply!
> >>>>>> 
> >>>>>> FWIW, the simplest fix would be something like below, just extending
> >>>>>> the mutex coverage.  But it'll serialize the all calls, so it might
> >>>>>> influence on the performance, while it's the safest way.
> >>>>> 
> >>>>> It may be better to update total_pcm_alloc_bytes before
> >>>>> snd_dma_alloc_dir_pages() call and decrease this value when allocation
> >>>>> fails to allow parallel allocations. Then the mutex can be held only
> >>>>> for the total_pcm_alloc_bytes variable update.
> >>>> 
> >>>> Yes, it'd work.  But a tricky part is that the actual allocation size
> >>>> can be bigger, and we need to correct the total_pcm_alloc_bytes after
> >>>> the allocation result.  So the end result would be a patch like below,
> >>>> which is a bit more complex than the previous simpler approach.  But
> >>>> it might be OK.
> >>> 
> >>> The patch looks good, but it may be better to move the "post" variable
> >>> updates to an inline function (mutex lock - update - mutex unlock) for
> >>> a better readability.
> >> 
> >> Sounds like a good idea.  Let me cook later.
> > 
> > ... and here it is.
> > 
> > If that looks OK, I'll submit a proper fix patch.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > --- a/sound/core/pcm_memory.c
> > +++ b/sound/core/pcm_memory.c
> > @@ -31,15 +31,41 @@ static unsigned long max_alloc_per_card = 32UL * 1024UL * 1024UL;
> >   module_param(max_alloc_per_card, ulong, 0644);
> >   MODULE_PARM_DESC(max_alloc_per_card, "Max total allocation bytes per card.");
> >   +static void __update_allocated_size(struct snd_card *card,
> > ssize_t bytes)
> 
> Missing inline ? May be also used for
> 
> > +static void update_allocated_size(struct snd_card *card, ssize_t bytes)
> > +static void decrease_allocated_size(struct snd_card *card, size_t bytes)

I left the optimizations to compilers.  Usually they do inline if it
makes sense, and it's often a more sensible choice.


thanks,

Takashi