| Message ID | 20200707180836.5435-1-vr_qemu@t-online.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ossaudio: fix out of bounds write | expand |
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.
> 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
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
> > 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
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;
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(+)