* Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
2018-10-30 8:23 [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size Gerd Hoffmann
@ 2018-10-30 8:45 ` Philippe Mathieu-Daudé
2018-10-31 5:02 ` no-reply
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-30 8:45 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel; +Cc: Prasad J Pandit
Hi Gerd,
On 30/10/18 9:23, Gerd Hoffmann wrote:
Can you add your previous patch description,
We have a lovely, guest-triggerable buffer overflow in opl2 emulation.
Reproducer:
outw(0xff60, 0x220);
outw(0x1020, 0x220);
outw(0xffb0, 0x220);
Result:
Will overflow FM_OPL->AR_TABLE[] (see hw/audio/fmopl.[ch])
And Prasad Pandit triggering flow:
In set_ar_dr
SLOT->AR = ar ? &OPL->AR_TABLE[ar<<2] : RATE_0;
SLOT->AR is set to point to OPL->DR_TABLE[60] and while so if s->ksr
is set to 15, in CALC_FCSLOT()
SLOT->evsa = SLOT->AR[ksr]; <= accesses OPL->AR_TABLE[60 + 15];
Thanks.
> Fixes: CVE-2018-???
> Cc: P J P <ppandit@redhat.com>
> Reported-by: Wangjunqing <wangjunqing@huawei.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/audio/fmopl.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/audio/fmopl.h b/hw/audio/fmopl.h
> index e7e578a48e..7199afaa3c 100644
> --- a/hw/audio/fmopl.h
> +++ b/hw/audio/fmopl.h
> @@ -72,8 +72,8 @@ typedef struct fm_opl_f {
> /* Rhythm sention */
> uint8_t rhythm; /* Rhythm mode , key flag */
> /* time tables */
> - int32_t AR_TABLE[75]; /* atttack rate tables */
> - int32_t DR_TABLE[75]; /* decay rate tables */
> + int32_t AR_TABLE[76]; /* atttack rate tables */
> + int32_t DR_TABLE[76]; /* decay rate tables */
> uint32_t FN_TABLE[1024]; /* fnumber -> increment counter */
> /* LFO */
> int32_t *ams_table;
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
2018-10-30 8:23 [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size Gerd Hoffmann
2018-10-30 8:45 ` Philippe Mathieu-Daudé
@ 2018-10-31 5:02 ` no-reply
2018-10-31 14:10 ` Philippe Mathieu-Daudé
2018-11-12 13:18 ` Gerd Hoffmann
2018-11-27 7:30 ` Thomas Huth
3 siblings, 1 reply; 8+ messages in thread
From: no-reply @ 2018-10-31 5:02 UTC (permalink / raw)
To: kraxel; +Cc: famz, qemu-devel, ppandit
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20181030082340.17170-1-kraxel@redhat.com
Subject: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c87f8e4c5c fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
=== OUTPUT BEGIN ===
Checking PATCH 1/1: fmops: fix off-by-one in AR_TABLE and DR_TABLE array size...
ERROR: code indent should never use tabs
#27: FILE: hw/audio/fmopl.h:75:
+^Iint32_t AR_TABLE[76];^I/* atttack rate tables */$
ERROR: code indent should never use tabs
#28: FILE: hw/audio/fmopl.h:76:
+^Iint32_t DR_TABLE[76];^I/* decay rate tables */$
total: 2 errors, 0 warnings, 10 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
2018-10-31 5:02 ` no-reply
@ 2018-10-31 14:10 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-31 14:10 UTC (permalink / raw)
To: qemu-devel, kraxel; +Cc: ppandit, famz
Hi Gerd,
On 31/10/18 6:02, no-reply@patchew.org wrote:
> === OUTPUT BEGIN ===
> Checking PATCH 1/1: fmops: fix off-by-one in AR_TABLE and DR_TABLE array size...
> ERROR: code indent should never use tabs
> #27: FILE: hw/audio/fmopl.h:75:
> +^Iint32_t AR_TABLE[76];^I/* atttack rate tables */$
Can you also fix "attack" when respinning?
>
> ERROR: code indent should never use tabs
> #28: FILE: hw/audio/fmopl.h:76:
> +^Iint32_t DR_TABLE[76];^I/* decay rate tables */$
>
> total: 2 errors, 0 warnings, 10 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> === OUTPUT END ===
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
2018-10-30 8:23 [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size Gerd Hoffmann
2018-10-30 8:45 ` Philippe Mathieu-Daudé
2018-10-31 5:02 ` no-reply
@ 2018-11-12 13:18 ` Gerd Hoffmann
2018-11-21 10:49 ` P J P
2018-11-27 7:30 ` Thomas Huth
3 siblings, 1 reply; 8+ messages in thread
From: Gerd Hoffmann @ 2018-11-12 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: P J P
On Tue, Oct 30, 2018 at 09:23:40AM +0100, Gerd Hoffmann wrote:
> Fixes: CVE-2018-???
> Cc: P J P <ppandit@redhat.com>
ping, do we have a cve number meanwhile?
cheers,
Gerd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
2018-11-12 13:18 ` Gerd Hoffmann
@ 2018-11-21 10:49 ` P J P
2018-11-21 13:07 ` Gerd Hoffmann
0 siblings, 1 reply; 8+ messages in thread
From: P J P @ 2018-11-21 10:49 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: qemu-devel
Hello Gerd,
+-- On Mon, 12 Nov 2018, Gerd Hoffmann wrote --+
| On Tue, Oct 30, 2018 at 09:23:40AM +0100, Gerd Hoffmann wrote:
| > Fixes: CVE-2018-???
| > Cc: P J P <ppandit@redhat.com>
|
| ping, do we have a cve number meanwhile?
No, the off-by-one does not seem to have an adverse effect. One byte past
AR_TABLE[75] array would likely read into DR_TABLE[75] array, which would
anyway be accessible to a driver. It does not seem to crash Qemu either. I
think it's more of a bug fix, than security fix. Hope that's okay.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
2018-11-21 10:49 ` P J P
@ 2018-11-21 13:07 ` Gerd Hoffmann
0 siblings, 0 replies; 8+ messages in thread
From: Gerd Hoffmann @ 2018-11-21 13:07 UTC (permalink / raw)
To: P J P; +Cc: qemu-devel
On Wed, Nov 21, 2018 at 04:19:11PM +0530, P J P wrote:
> Hello Gerd,
>
> +-- On Mon, 12 Nov 2018, Gerd Hoffmann wrote --+
> | On Tue, Oct 30, 2018 at 09:23:40AM +0100, Gerd Hoffmann wrote:
> | > Fixes: CVE-2018-???
> | > Cc: P J P <ppandit@redhat.com>
> |
> | ping, do we have a cve number meanwhile?
>
> No, the off-by-one does not seem to have an adverse effect. One byte past
> AR_TABLE[75] array would likely read into DR_TABLE[75] array, which would
> anyway be accessible to a driver. It does not seem to crash Qemu either. I
> think it's more of a bug fix, than security fix. Hope that's okay.
Ok, makes sense, I'll drop the cve line then and queue the patch.
cheers,
Gerd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
2018-10-30 8:23 [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size Gerd Hoffmann
` (2 preceding siblings ...)
2018-11-12 13:18 ` Gerd Hoffmann
@ 2018-11-27 7:30 ` Thomas Huth
3 siblings, 0 replies; 8+ messages in thread
From: Thomas Huth @ 2018-11-27 7:30 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel; +Cc: P J P, qemu-stable
On 2018-10-30 09:23, Gerd Hoffmann wrote:
> Fixes: CVE-2018-???
> Cc: P J P <ppandit@redhat.com>
> Reported-by: Wangjunqing <wangjunqing@huawei.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/audio/fmopl.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/audio/fmopl.h b/hw/audio/fmopl.h
> index e7e578a48e..7199afaa3c 100644
> --- a/hw/audio/fmopl.h
> +++ b/hw/audio/fmopl.h
> @@ -72,8 +72,8 @@ typedef struct fm_opl_f {
> /* Rhythm sention */
> uint8_t rhythm; /* Rhythm mode , key flag */
> /* time tables */
> - int32_t AR_TABLE[75]; /* atttack rate tables */
> - int32_t DR_TABLE[75]; /* decay rate tables */
> + int32_t AR_TABLE[76]; /* atttack rate tables */
> + int32_t DR_TABLE[76]; /* decay rate tables */
> uint32_t FN_TABLE[1024]; /* fnumber -> increment counter */
> /* LFO */
> int32_t *ams_table;
>
CC to qemu-stable ?
Thomas
^ permalink raw reply [flat|nested] 8+ messages in thread