diff mbox

Fix for possible null pointer dereference in dma.c

Message ID 1400191076-28279-1-git-send-email-rickard_strandqvist@spectrumdigital.se (mailing list archive)
State New, archived
Headers show

Commit Message

Rickard Strandqvist May 15, 2014, 9:57 p.m. UTC
There is otherwise a risk of a possible null pointer dereference.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 sound/soc/samsung/dma.c |   10 ++++++----
 1 fil ändrad, 6 tillägg(+), 4 borttagningar(-)

Comments

Lothar Waßmann May 19, 2014, 5:39 a.m. UTC | #1
Hi,

Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  sound/soc/samsung/dma.c |   10 ++++++----
>  1 fil ändrad, 6 tillägg(+), 4 borttagningar(-)
> 
> diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
> index dc09b71..b1f6757 100644
> --- a/sound/soc/samsung/dma.c
> +++ b/sound/soc/samsung/dma.c
> @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream)
>  static void audio_buffdone(void *data)
>  {
>  	struct snd_pcm_substream *substream = data;
> -	struct runtime_data *prtd = substream->runtime->private_data;
> +	struct runtime_data *prtd = NULL;
>  
>  	pr_debug("Entered %s\n", __func__);
>  
> -	if (prtd->state & ST_RUNNING) {
> +	if(substream)
>
Missing space.


Lothar Waßmann
Tomasz Figa May 19, 2014, 10:52 a.m. UTC | #2
Hi Rickard,

On 15.05.2014 23:57, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  sound/soc/samsung/dma.c |   10 ++++++----
>  1 fil ändrad, 6 tillägg(+), 4 borttagningar(-)
> 
> diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
> index dc09b71..b1f6757 100644
> --- a/sound/soc/samsung/dma.c
> +++ b/sound/soc/samsung/dma.c
> @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream)
>  static void audio_buffdone(void *data)
>  {
>  	struct snd_pcm_substream *substream = data;
> -	struct runtime_data *prtd = substream->runtime->private_data;
> +	struct runtime_data *prtd = NULL;
>  
>  	pr_debug("Entered %s\n", __func__);
>  
> -	if (prtd->state & ST_RUNNING) {
> +	if(substream)
> +		prtd = substream->runtime->private_data;
> +
> +	if (prtd && prtd->state & ST_RUNNING) {
>  		prtd->dma_pos += prtd->dma_period;
>  		if (prtd->dma_pos >= prtd->dma_end)
>  			prtd->dma_pos = prtd->dma_start;
>  
> -		if (substream)
> -			snd_pcm_period_elapsed(substream);
> +		snd_pcm_period_elapsed(substream);
>  
>  		spin_lock(&prtd->lock);
>  		if (!samsung_dma_has_circular()) {
> 

Can you find a path (or use case, if possible) on which any of affected
pointers will be NULL?

Best regards,
Tomasz
Rickard Strandqvist May 20, 2014, 7:12 p.m. UTC | #3
Hi Tomasz

What I based my patch on is really because of this line:
if (substream)
         snd_pcm_period_elapsed(substream);

Boojin Kim thought that this was needed, if this is true anymore..? I
have not been able to immerse myself so much in all patches.
I'm working on about 100 similar patches.

344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
09:44:43 +0900 123)              prtd->dma_pos += prtd->dma_period;
344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
09:44:43 +0900 124)              if (prtd->dma_pos >= prtd->dma_end)
344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
09:44:43 +0900 125)                      prtd->dma_pos =
prtd->dma_start;
5111c075 sound/soc/s3c24xx/s3c24xx-pcm.c (Mark Brown        2008-04-30
17:19:32 +0200 126)
344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
09:44:43 +0900 127)              if (substream)
344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
09:44:43 +0900 128)
snd_pcm_period_elapsed(substream);
c0f41bb1 sound/soc/s3c24xx/s3c24xx-pcm.c (Ben Dooks         2007-02-14
13:20:03 +0100 129)
344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
09:44:43 +0900 130)              spin_lock(&prtd->lock);


Best regards
Rickard Strandqvist


2014-05-19 12:52 GMT+02:00 Tomasz Figa <tomasz.figa@gmail.com>:
> Hi Rickard,
>
> On 15.05.2014 23:57, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  sound/soc/samsung/dma.c |   10 ++++++----
>>  1 fil ändrad, 6 tillägg(+), 4 borttagningar(-)
>>
>> diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
>> index dc09b71..b1f6757 100644
>> --- a/sound/soc/samsung/dma.c
>> +++ b/sound/soc/samsung/dma.c
>> @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream)
>>  static void audio_buffdone(void *data)
>>  {
>>       struct snd_pcm_substream *substream = data;
>> -     struct runtime_data *prtd = substream->runtime->private_data;
>> +     struct runtime_data *prtd = NULL;
>>
>>       pr_debug("Entered %s\n", __func__);
>>
>> -     if (prtd->state & ST_RUNNING) {
>> +     if(substream)
>> +             prtd = substream->runtime->private_data;
>> +
>> +     if (prtd && prtd->state & ST_RUNNING) {
>>               prtd->dma_pos += prtd->dma_period;
>>               if (prtd->dma_pos >= prtd->dma_end)
>>                       prtd->dma_pos = prtd->dma_start;
>>
>> -             if (substream)
>> -                     snd_pcm_period_elapsed(substream);
>> +             snd_pcm_period_elapsed(substream);
>>
>>               spin_lock(&prtd->lock);
>>               if (!samsung_dma_has_circular()) {
>>
>
> Can you find a path (or use case, if possible) on which any of affected
> pointers will be NULL?
>
> Best regards,
> Tomasz
Tomasz Figa May 20, 2014, 7:16 p.m. UTC | #4
Hi Rickard,

On 20.05.2014 21:12, Rickard Strandqvist wrote:
> Hi Tomasz
> 
> What I based my patch on is really because of this line:
> if (substream)
>          snd_pcm_period_elapsed(substream);
> 
> Boojin Kim thought that this was needed, if this is true anymore..? I
> have not been able to immerse myself so much in all patches.
> I'm working on about 100 similar patches.

To me having NULL as either data argument of buffer done callback or
private_data would be a serious driver bug and IMHO it's better to let
it crash with a NULL pointer dereference to let someone notice than mask
it by adding a condition.

Still, I'm not too experienced with ALSA and ASoC, so I might be wrong.
Mark, what do you think about this?

Best regards,
Tomasz

> 
> 344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
> 09:44:43 +0900 123)              prtd->dma_pos += prtd->dma_period;
> 344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
> 09:44:43 +0900 124)              if (prtd->dma_pos >= prtd->dma_end)
> 344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
> 09:44:43 +0900 125)                      prtd->dma_pos =
> prtd->dma_start;
> 5111c075 sound/soc/s3c24xx/s3c24xx-pcm.c (Mark Brown        2008-04-30
> 17:19:32 +0200 126)
> 344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
> 09:44:43 +0900 127)              if (substream)
> 344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
> 09:44:43 +0900 128)
> snd_pcm_period_elapsed(substream);
> c0f41bb1 sound/soc/s3c24xx/s3c24xx-pcm.c (Ben Dooks         2007-02-14
> 13:20:03 +0100 129)
> 344b4c48 sound/soc/samsung/dma.c         (Boojin Kim        2011-09-02
> 09:44:43 +0900 130)              spin_lock(&prtd->lock);
> 
> 
> Best regards
> Rickard Strandqvist
> 
> 
> 2014-05-19 12:52 GMT+02:00 Tomasz Figa <tomasz.figa@gmail.com>:
>> Hi Rickard,
>>
>> On 15.05.2014 23:57, Rickard Strandqvist wrote:
>>> There is otherwise a risk of a possible null pointer dereference.
>>>
>>> Was largely found by using a static code analysis program called cppcheck.
>>>
>>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>>> ---
>>>  sound/soc/samsung/dma.c |   10 ++++++----
>>>  1 fil ändrad, 6 tillägg(+), 4 borttagningar(-)
>>>
>>> diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
>>> index dc09b71..b1f6757 100644
>>> --- a/sound/soc/samsung/dma.c
>>> +++ b/sound/soc/samsung/dma.c
>>> @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream)
>>>  static void audio_buffdone(void *data)
>>>  {
>>>       struct snd_pcm_substream *substream = data;
>>> -     struct runtime_data *prtd = substream->runtime->private_data;
>>> +     struct runtime_data *prtd = NULL;
>>>
>>>       pr_debug("Entered %s\n", __func__);
>>>
>>> -     if (prtd->state & ST_RUNNING) {
>>> +     if(substream)
>>> +             prtd = substream->runtime->private_data;
>>> +
>>> +     if (prtd && prtd->state & ST_RUNNING) {
>>>               prtd->dma_pos += prtd->dma_period;
>>>               if (prtd->dma_pos >= prtd->dma_end)
>>>                       prtd->dma_pos = prtd->dma_start;
>>>
>>> -             if (substream)
>>> -                     snd_pcm_period_elapsed(substream);
>>> +             snd_pcm_period_elapsed(substream);
>>>
>>>               spin_lock(&prtd->lock);
>>>               if (!samsung_dma_has_circular()) {
>>>
>>
>> Can you find a path (or use case, if possible) on which any of affected
>> pointers will be NULL?
>>
>> Best regards,
>> Tomasz
Andrew Eikum May 20, 2014, 7:29 p.m. UTC | #5
On Tue, May 20, 2014 at 09:16:59PM +0200, Tomasz Figa wrote:
> To me having NULL as either data argument of buffer done callback or
> private_data would be a serious driver bug and IMHO it's better to let
> it crash with a NULL pointer dereference to let someone notice than mask
> it by adding a condition.
> 

Please, no.  Regardless of who is at fault, crashes in library code
are insanely frustrating to application developers.  Error codes can
be handled nicely and expressed to the user as an error message.
Crashes are hideous for end users and usually end up being support
nightmares for developers.

Related:
https://git.kernel.org/cgit/linux/kernel/git/kay/libabc.git/tree/README?id=0573b57c7bdb4c82d6acd62f7923a6503f4f679a#n126

Andrew
Tomasz Figa May 20, 2014, 7:31 p.m. UTC | #6
On 20.05.2014 21:29, Andrew Eikum wrote:
> On Tue, May 20, 2014 at 09:16:59PM +0200, Tomasz Figa wrote:
>> To me having NULL as either data argument of buffer done callback or
>> private_data would be a serious driver bug and IMHO it's better to let
>> it crash with a NULL pointer dereference to let someone notice than mask
>> it by adding a condition.
>>
> 
> Please, no.  Regardless of who is at fault, crashes in library code
> are insanely frustrating to application developers.  Error codes can
> be handled nicely and expressed to the user as an error message.
> Crashes are hideous for end users and usually end up being support
> nightmares for developers.

How is the related code a library?

Best regards,
Tomasz
Lars-Peter Clausen May 20, 2014, 9:21 p.m. UTC | #7
On 05/20/2014 09:16 PM, Tomasz Figa wrote:
> Hi Rickard,
>
> On 20.05.2014 21:12, Rickard Strandqvist wrote:
>> Hi Tomasz
>>
>> What I based my patch on is really because of this line:
>> if (substream)
>>           snd_pcm_period_elapsed(substream);
>>
>> Boojin Kim thought that this was needed, if this is true anymore..? I
>> have not been able to immerse myself so much in all patches.
>> I'm working on about 100 similar patches.
>
> To me having NULL as either data argument of buffer done callback or
> private_data would be a serious driver bug and IMHO it's better to let
> it crash with a NULL pointer dereference to let someone notice than mask
> it by adding a condition.
>
> Still, I'm not too experienced with ALSA and ASoC, so I might be wrong.
> Mark, what do you think about this?

Given that there is a patch[1] which removes the whole file I think we can stop 
the discussion about this patch here.

But for the record, substream will never ever be NULL in this function. prtd 
might be though if the DMA completion callback races against the closing of the 
PCM stream.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076860.html
Mark Brown May 20, 2014, 9:58 p.m. UTC | #8
On Tue, May 20, 2014 at 11:21:08PM +0200, Lars-Peter Clausen wrote:
> On 05/20/2014 09:16 PM, Tomasz Figa wrote:

> >Still, I'm not too experienced with ALSA and ASoC, so I might be wrong.
> >Mark, what do you think about this?

> Given that there is a patch[1] which removes the whole file I think we can
> stop the discussion about this patch here.

> But for the record, substream will never ever be NULL in this function. prtd
> might be though if the DMA completion callback races against the closing of
> the PCM stream.

Yeah, what Lars said - obviously the patch removing the driver is more
comprehensive!

For the record it's very helpful to be specific about what the problem
being addressed is in the changelog, if it's not obvious what the bug
that's being fixed is that'll slow things down.  Please also use subject
lines appropriate for the subsystem, it will help your patch get
noticed.
diff mbox

Patch

diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
index dc09b71..b1f6757 100644
--- a/sound/soc/samsung/dma.c
+++ b/sound/soc/samsung/dma.c
@@ -115,17 +115,19 @@  static void dma_enqueue(struct snd_pcm_substream *substream)
 static void audio_buffdone(void *data)
 {
 	struct snd_pcm_substream *substream = data;
-	struct runtime_data *prtd = substream->runtime->private_data;
+	struct runtime_data *prtd = NULL;
 
 	pr_debug("Entered %s\n", __func__);
 
-	if (prtd->state & ST_RUNNING) {
+	if(substream)
+		prtd = substream->runtime->private_data;
+
+	if (prtd && prtd->state & ST_RUNNING) {
 		prtd->dma_pos += prtd->dma_period;
 		if (prtd->dma_pos >= prtd->dma_end)
 			prtd->dma_pos = prtd->dma_start;
 
-		if (substream)
-			snd_pcm_period_elapsed(substream);
+		snd_pcm_period_elapsed(substream);
 
 		spin_lock(&prtd->lock);
 		if (!samsung_dma_has_circular()) {