* Re: [PATCH] ossaudio: fix out of bounds write
2020-07-08 8:30 ` Philippe Mathieu-Daudé
@ 2020-07-08 20:09 ` Volker Rümelin
2020-07-09 1:08 ` BALATON Zoltan
2020-07-09 12:55 ` Gerd Hoffmann
2 siblings, 0 replies; 5+ messages in thread
From: Volker Rümelin @ 2020-07-08 20:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Gerd Hoffmann
Cc: QEMU, Zoltán Kővágó
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ossaudio: fix out of bounds write
2020-07-08 8:30 ` Philippe Mathieu-Daudé
2020-07-08 20:09 ` Volker Rümelin
@ 2020-07-09 1:08 ` BALATON Zoltan
2020-07-09 12:55 ` Gerd Hoffmann
2 siblings, 0 replies; 5+ messages in thread
From: BALATON Zoltan @ 2020-07-09 1:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Zoltán Kővágó, Volker Rümelin,
Gerd Hoffmann, QEMU
[-- 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] ossaudio: fix out of bounds write
2020-07-08 8:30 ` Philippe Mathieu-Daudé
2020-07-08 20:09 ` Volker Rümelin
2020-07-09 1:08 ` BALATON Zoltan
@ 2020-07-09 12:55 ` Gerd Hoffmann
2 siblings, 0 replies; 5+ messages in thread
From: Gerd Hoffmann @ 2020-07-09 12:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Volker Rümelin, QEMU, Zoltán Kővágó
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread