[Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[Gerd Hoffmann]

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;
-- 
2.9.3

Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[Philippe Mathieu-Daudé]

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;
> 

Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[no-reply]

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

Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[Philippe Mathieu-Daudé]

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 ===

Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[P J P]

 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

Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size

[Thomas Huth]

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