[PATCH] ossaudio: fix out of bounds write

[PATCH] ossaudio: fix out of bounds write

[Volker Rümelin]

In function oss_read() a read error currently does not exit the
read loop. With no data to read the variable pos will quickly
underflow and a subsequent successful read overwrites memory
outside the buffer. This patch adds the missing break statement
to the error path of the function.

To reproduce start qemu with -audiodev oss,id=audio0 and in the
guest start audio recording. After some time this will trigger
an exception.

Fixes: 3ba4066d08 "ossaudio: port to the new audio backend api"

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/ossaudio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/audio/ossaudio.c b/audio/ossaudio.c
index f88d076ec2..a7dcaa31ad 100644
--- a/audio/ossaudio.c
+++ b/audio/ossaudio.c
@@ -691,6 +691,7 @@ static size_t oss_read(HWVoiceIn *hw, void *buf, size_t len)
                            len, dst);
                 break;
             }
+            break;
         }
 
         pos += nread;
-- 
2.26.2

Re: [PATCH] ossaudio: fix out of bounds write

[Philippe Mathieu-Daudé]

On 7/7/20 8:08 PM, Volker Rümelin wrote:
> In function oss_read() a read error currently does not exit the
> read loop. With no data to read the variable pos will quickly
> underflow and a subsequent successful read overwrites memory
> outside the buffer. This patch adds the missing break statement
> to the error path of the function.

Correct, but ...

> 
> To reproduce start qemu with -audiodev oss,id=audio0 and in the
> guest start audio recording. After some time this will trigger
> an exception.
> 
> Fixes: 3ba4066d08 "ossaudio: port to the new audio backend api"
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  audio/ossaudio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/audio/ossaudio.c b/audio/ossaudio.c
> index f88d076ec2..a7dcaa31ad 100644
> --- a/audio/ossaudio.c
> +++ b/audio/ossaudio.c
> @@ -691,6 +691,7 @@ static size_t oss_read(HWVoiceIn *hw, void *buf, size_t len)
>                             len, dst);
>                  break;
>              }
> +            break;
>          }
>  
>          pos += nread;

... now pos += -1, then the size returned misses the last byte.

Re: [PATCH] ossaudio: fix out of bounds write

[Volker Rümelin]

> On 7/7/20 8:08 PM, Volker Rümelin wrote:
>> In function oss_read() a read error currently does not exit the
>> read loop. With no data to read the variable pos will quickly
>> underflow and a subsequent successful read overwrites memory
>> outside the buffer. This patch adds the missing break statement
>> to the error path of the function.
> Correct, but ...
>
>> To reproduce start qemu with -audiodev oss,id=audio0 and in the
>> guest start audio recording. After some time this will trigger
>> an exception.
>>
>> Fixes: 3ba4066d08 "ossaudio: port to the new audio backend api"
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>  audio/ossaudio.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/audio/ossaudio.c b/audio/ossaudio.c
>> index f88d076ec2..a7dcaa31ad 100644
>> --- a/audio/ossaudio.c
>> +++ b/audio/ossaudio.c
>> @@ -691,6 +691,7 @@ static size_t oss_read(HWVoiceIn *hw, void *buf, size_t len)
>>                             len, dst);
>>                  break;
>>              }
>> +            break;
>>          }
>>  
>>          pos += nread;
> ... now pos += -1, then the size returned misses the last byte.
>
Hi Philippe,

no, the added break breaks the while loop. The next executed instruction after this break is the return pos statement not pos += nread.

With best regards,
Volker

Re: [PATCH] ossaudio: fix out of bounds write

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

On Wed, 8 Jul 2020, Philippe Mathieu-Daudé wrote:
> On 7/7/20 8:08 PM, Volker Rümelin wrote:
>> In function oss_read() a read error currently does not exit the
>> read loop. With no data to read the variable pos will quickly
>> underflow and a subsequent successful read overwrites memory
>> outside the buffer. This patch adds the missing break statement
>> to the error path of the function.
>
> Correct, but ...
>
>>
>> To reproduce start qemu with -audiodev oss,id=audio0 and in the
>> guest start audio recording. After some time this will trigger
>> an exception.
>>
>> Fixes: 3ba4066d08 "ossaudio: port to the new audio backend api"
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>  audio/ossaudio.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/audio/ossaudio.c b/audio/ossaudio.c
>> index f88d076ec2..a7dcaa31ad 100644
>> --- a/audio/ossaudio.c
>> +++ b/audio/ossaudio.c
>> @@ -691,6 +691,7 @@ static size_t oss_read(HWVoiceIn *hw, void *buf, size_t len)
>>                             len, dst);
>>                  break;
>>              }
>> +            break;

Maybe it would be less confusing if you converted the switch(errno) to an 
if then you wouldn't have two senses of break; in close proximity. I was 
thinking something like

if (nread == -1) {
     if (errno != EINTR && errno != EAGAIN) {
         logerr();
     }
     break; /* from while, which is now clear */
}

>>          }
>>
>>          pos += nread;
>
> ... now pos += -1, then the size returned misses the last byte.

Don't you break from loop in if () above this so -1 is never added to pos 
after this patch? What happens for EINTR and EAGAIN? Now we break from the 
loop for those too, should it be continue; instead?

Regards,
BALATON Zoltan

Re: [PATCH] ossaudio: fix out of bounds write

[Gerd Hoffmann]

> > diff --git a/audio/ossaudio.c b/audio/ossaudio.c
> > index f88d076ec2..a7dcaa31ad 100644
> > --- a/audio/ossaudio.c
> > +++ b/audio/ossaudio.c
> > @@ -691,6 +691,7 @@ static size_t oss_read(HWVoiceIn *hw, void *buf, size_t len)
> >                             len, dst);
> >                  break;
> >              }
> > +            break;
> >          }
> >  
> >          pos += nread;
> 
> ... now pos += -1, then the size returned misses the last byte.

No, it doesn't.  break leaves the while loop, not the if condition.
From patch context it isn't obvious though, you need to look at the
source code ...

Patch queued.

thanks,
  Gerd