* [Qemu-devel] [PATCH] vt82c686: avoid out-of-bounds read
@ 2014-12-10 9:17 ` Paolo Bonzini
0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2014-12-10 9:17 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, zhang.zhanghailiang, peter.huangpeng
superio_ioport_readb can read the 256th element of the array.
Coverity reports an out-of-bounds write in superio_ioport_writeb,
but it does not show the corresponding out-of-bounds read
because it cannot prove that it can happen. Fix the root
cause of the problem (zhanghailang's patch instead fixes
the logic in superio_ioport_writeb).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/isa/vt82c686.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index e0c235c..a43e26d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -36,7 +36,7 @@
typedef struct SuperIOConfig
{
- uint8_t config[0xff];
+ uint8_t config[0x100];
uint8_t index;
uint8_t data;
} SuperIOConfig;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
2014-12-10 9:17 ` [Qemu-devel] " Paolo Bonzini
@ 2014-12-10 9:31 ` zhanghailiang
-1 siblings, 0 replies; 14+ messages in thread
From: zhanghailiang @ 2014-12-10 9:31 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-trivial, peter.huangpeng
Hi paolo,
Will this change affects the migration?
I noticed that there is a member 'SuperIOConfig superio_conf' in VT82C686BState.
vt82c686 seems only to be used in mips64el target, Do we support migration for mips target?
Thanks,
zhanghailiang
On 2014/12/10 17:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
> Coverity reports an out-of-bounds write in superio_ioport_writeb,
> but it does not show the corresponding out-of-bounds read
> because it cannot prove that it can happen. Fix the root
> cause of the problem (zhanghailang's patch instead fixes
> the logic in superio_ioport_writeb).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/isa/vt82c686.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index e0c235c..a43e26d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -36,7 +36,7 @@
>
> typedef struct SuperIOConfig
> {
> - uint8_t config[0xff];
> + uint8_t config[0x100];
> uint8_t index;
> uint8_t data;
> } SuperIOConfig;
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH] vt82c686: avoid out-of-bounds read
@ 2014-12-10 9:31 ` zhanghailiang
0 siblings, 0 replies; 14+ messages in thread
From: zhanghailiang @ 2014-12-10 9:31 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-trivial, peter.huangpeng
Hi paolo,
Will this change affects the migration?
I noticed that there is a member 'SuperIOConfig superio_conf' in VT82C686BState.
vt82c686 seems only to be used in mips64el target, Do we support migration for mips target?
Thanks,
zhanghailiang
On 2014/12/10 17:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
> Coverity reports an out-of-bounds write in superio_ioport_writeb,
> but it does not show the corresponding out-of-bounds read
> because it cannot prove that it can happen. Fix the root
> cause of the problem (zhanghailang's patch instead fixes
> the logic in superio_ioport_writeb).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/isa/vt82c686.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index e0c235c..a43e26d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -36,7 +36,7 @@
>
> typedef struct SuperIOConfig
> {
> - uint8_t config[0xff];
> + uint8_t config[0x100];
> uint8_t index;
> uint8_t data;
> } SuperIOConfig;
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
2014-12-10 9:31 ` [Qemu-devel] " zhanghailiang
@ 2014-12-10 9:52 ` Paolo Bonzini
-1 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2014-12-10 9:52 UTC (permalink / raw)
To: zhanghailiang, qemu-devel; +Cc: qemu-trivial, peter.huangpeng
On 10/12/2014 10:31, zhanghailiang wrote:
> Hi paolo,
>
> Will this change affects the migration?
> I noticed that there is a member 'SuperIOConfig superio_conf' in
> VT82C686BState.
>
> vt82c686 seems only to be used in mips64el target, Do we support
> migration for mips target?
No, there is no VMState for superio_conf.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
2014-12-10 9:17 ` [Qemu-devel] " Paolo Bonzini
@ 2014-12-11 8:35 ` zhanghailiang
-1 siblings, 0 replies; 14+ messages in thread
From: zhanghailiang @ 2014-12-11 8:35 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-trivial, peter.huangpeng
On 2014/12/10 17:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
> Coverity reports an out-of-bounds write in superio_ioport_writeb,
> but it does not show the corresponding out-of-bounds read
> because it cannot prove that it can happen. Fix the root
> cause of the problem (zhanghailang's patch instead fixes
> the logic in superio_ioport_writeb).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/isa/vt82c686.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index e0c235c..a43e26d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -36,7 +36,7 @@
>
> typedef struct SuperIOConfig
> {
> - uint8_t config[0xff];
> + uint8_t config[0x100];
> uint8_t index;
> uint8_t data;
> } SuperIOConfig;
>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH] vt82c686: avoid out-of-bounds read
@ 2014-12-11 8:35 ` zhanghailiang
0 siblings, 0 replies; 14+ messages in thread
From: zhanghailiang @ 2014-12-11 8:35 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-trivial, peter.huangpeng
On 2014/12/10 17:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
> Coverity reports an out-of-bounds write in superio_ioport_writeb,
> but it does not show the corresponding out-of-bounds read
> because it cannot prove that it can happen. Fix the root
> cause of the problem (zhanghailang's patch instead fixes
> the logic in superio_ioport_writeb).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/isa/vt82c686.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index e0c235c..a43e26d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -36,7 +36,7 @@
>
> typedef struct SuperIOConfig
> {
> - uint8_t config[0xff];
> + uint8_t config[0x100];
> uint8_t index;
> uint8_t data;
> } SuperIOConfig;
>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
2014-12-10 9:17 ` [Qemu-devel] " Paolo Bonzini
@ 2014-12-11 17:55 ` Michael Tokarev
-1 siblings, 0 replies; 14+ messages in thread
From: Michael Tokarev @ 2014-12-11 17:55 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: qemu-trivial, zhang.zhanghailiang, peter.huangpeng
10.12.2014 12:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
Is there a legitimate reason for it to access byte index 256?
What is the actual size of superio config memory, 256 or 257?
I don't know, but somehow it looks like it should be 256.
If that's the case, the fix is wrong and superio_ioport_readb()
should be fixed instead...
Thanks,
/mjt
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
@ 2014-12-11 17:55 ` Michael Tokarev
0 siblings, 0 replies; 14+ messages in thread
From: Michael Tokarev @ 2014-12-11 17:55 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: qemu-trivial, zhang.zhanghailiang, peter.huangpeng
10.12.2014 12:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
Is there a legitimate reason for it to access byte index 256?
What is the actual size of superio config memory, 256 or 257?
I don't know, but somehow it looks like it should be 256.
If that's the case, the fix is wrong and superio_ioport_readb()
should be fixed instead...
Thanks,
/mjt
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
2014-12-11 17:55 ` [Qemu-devel] " Michael Tokarev
@ 2014-12-11 19:05 ` Paolo Bonzini
-1 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2014-12-11 19:05 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel
Cc: qemu-trivial, zhang.zhanghailiang, peter.huangpeng
On 11/12/2014 18:55, Michael Tokarev wrote:
>> > superio_ioport_readb can read the 256th element of the array.
> Is there a legitimate reason for it to access byte index 256?
The 256th element is byte index 255. :)
> What is the actual size of superio config memory, 256 or 257?
It's 256 and the array is sized conf[0xff].
> I don't know, but somehow it looks like it should be 256.
That's what the patch does. :)
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
@ 2014-12-11 19:05 ` Paolo Bonzini
0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2014-12-11 19:05 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel
Cc: qemu-trivial, zhang.zhanghailiang, peter.huangpeng
On 11/12/2014 18:55, Michael Tokarev wrote:
>> > superio_ioport_readb can read the 256th element of the array.
> Is there a legitimate reason for it to access byte index 256?
The 256th element is byte index 255. :)
> What is the actual size of superio config memory, 256 or 257?
It's 256 and the array is sized conf[0xff].
> I don't know, but somehow it looks like it should be 256.
That's what the patch does. :)
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
2014-12-10 9:17 ` [Qemu-devel] " Paolo Bonzini
@ 2015-01-12 8:49 ` Michael Tokarev
-1 siblings, 0 replies; 14+ messages in thread
From: Michael Tokarev @ 2015-01-12 8:49 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: qemu-trivial, zhang.zhanghailiang, peter.huangpeng
10.12.2014 12:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
> Coverity reports an out-of-bounds write in superio_ioport_writeb,
> but it does not show the corresponding out-of-bounds read
> because it cannot prove that it can happen. Fix the root
> cause of the problem (zhanghailang's patch instead fixes
> the logic in superio_ioport_writeb).
(Finally) applied to -trivial, thanks!
/mjt
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] vt82c686: avoid out-of-bounds read
@ 2015-01-12 8:49 ` Michael Tokarev
0 siblings, 0 replies; 14+ messages in thread
From: Michael Tokarev @ 2015-01-12 8:49 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: qemu-trivial, zhang.zhanghailiang, peter.huangpeng
10.12.2014 12:17, Paolo Bonzini wrote:
> superio_ioport_readb can read the 256th element of the array.
> Coverity reports an out-of-bounds write in superio_ioport_writeb,
> but it does not show the corresponding out-of-bounds read
> because it cannot prove that it can happen. Fix the root
> cause of the problem (zhanghailang's patch instead fixes
> the logic in superio_ioport_writeb).
(Finally) applied to -trivial, thanks!
/mjt
^ permalink raw reply [flat|nested] 14+ messages in thread