[Qemu-devel] [PATCH 0/2] memset/sizeof abuse

[Qemu-devel] [PATCH 0/2] memset/sizeof abuse

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

I ran coverity on all of qemu and have begun going through the results.
A couple problems jumped out as obvious and easy to fix:

Jim Meyering (2):
  kvm/apic: correct short memset
  cadence_gem: avoid stack-writing buffer-overrun

 hw/cadence_gem.c | 2 +-
 hw/kvm/apic.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--
1.7.10.1.487.ga3935e6

[Qemu-devel] [PATCH 1/2] kvm/apic: correct short memset

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

kvm_put_apic_state's attempt to clear *kapic before setting its
bits cleared sizeof(void*) bytes (no more than 8) rather than the
intended 1024 (KVM_APIC_REG_SIZE) bytes. Spotted by coverity.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/kvm/apic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
index ffe7a52..a0ab503 100644
--- a/hw/kvm/apic.c
+++ b/hw/kvm/apic.c
@@ -29,7 +29,7 @@ void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
     APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
     int i;

-    memset(kapic, 0, sizeof(kapic));
+    memset(kapic, 0, sizeof(*kapic));
     kvm_apic_set_reg(kapic, 0x2, s->id << 24);
     kvm_apic_set_reg(kapic, 0x8, s->tpr);
     kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24);
-- 
1.7.10.1.487.ga3935e6

[Qemu-devel] [PATCH 2/2] cadence_gem: avoid stack-writing buffer-overrun

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Use sizeof(rxbuf)-size (not sizeof(rxbuf-size)) as the number
of bytes to clear.  The latter would always clear 4 or 8
bytes, possibly writing beyond the end of that stack buffer.
Alternatively, depending on the value of the "size" parameter,
it could fail to initialize the end of "rxbuf".
Spotted by coverity.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 hw/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/cadence_gem.c b/hw/cadence_gem.c
index e2140ae..dbde392 100644
--- a/hw/cadence_gem.c
+++ b/hw/cadence_gem.c
@@ -664,7 +664,7 @@ static ssize_t gem_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
          */

         memcpy(rxbuf, buf, size);
-        memset(rxbuf + size, 0, sizeof(rxbuf - size));
+        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
         rxbuf_ptr = rxbuf;
         crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
         if (size < 60) {
-- 
1.7.10.1.487.ga3935e6

Re: [Qemu-devel] [PATCH 1/2] kvm/apic: correct short memset

[Jan Kiszka]

On 2012-05-10 13:19, Jim Meyering wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> kvm_put_apic_state's attempt to clear *kapic before setting its
> bits cleared sizeof(void*) bytes (no more than 8) rather than the
> intended 1024 (KVM_APIC_REG_SIZE) bytes. Spotted by coverity.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  hw/kvm/apic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
> index ffe7a52..a0ab503 100644
> --- a/hw/kvm/apic.c
> +++ b/hw/kvm/apic.c
> @@ -29,7 +29,7 @@ void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
>      APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
>      int i;
> 
> -    memset(kapic, 0, sizeof(kapic));
> +    memset(kapic, 0, sizeof(*kapic));
>      kvm_apic_set_reg(kapic, 0x2, s->id << 24);
>      kvm_apic_set_reg(kapic, 0x8, s->tpr);
>      kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24);

Yep, that's what I actually meant...

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/2] cadence_gem: avoid stack-writing buffer-overrun

[Peter Crosthwaite]

ACK and Thanks Jim,

Reviewed-by: Peter A.G. Crosthwaite <peter.crosthwaite@petalogix.com>

On Fri, May 11, 2012 at 2:19 AM, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
> Use sizeof(rxbuf)-size (not sizeof(rxbuf-size)) as the number
> of bytes to clear.  The latter would always clear 4 or 8
> bytes, possibly writing beyond the end of that stack buffer.
> Alternatively, depending on the value of the "size" parameter,
> it could fail to initialize the end of "rxbuf".
> Spotted by coverity.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  hw/cadence_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/cadence_gem.c b/hw/cadence_gem.c
> index e2140ae..dbde392 100644
> --- a/hw/cadence_gem.c
> +++ b/hw/cadence_gem.c
> @@ -664,7 +664,7 @@ static ssize_t gem_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
>          */
>
>         memcpy(rxbuf, buf, size);
> -        memset(rxbuf + size, 0, sizeof(rxbuf - size));
> +        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
>         rxbuf_ptr = rxbuf;
>         crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
>         if (size < 60) {
> --
> 1.7.10.1.487.ga3935e6
>

Re: [Qemu-devel] [PATCH 1.1 1/2] kvm/apic: correct short memset

[Stefan Weil]

Am 10.05.2012 19:28, schrieb Jan Kiszka:
> On 2012-05-10 13:19, Jim Meyering wrote:
>    
>> From: Jim Meyering<meyering@redhat.com>
>>
>> kvm_put_apic_state's attempt to clear *kapic before setting its
>> bits cleared sizeof(void*) bytes (no more than 8) rather than the
>> intended 1024 (KVM_APIC_REG_SIZE) bytes. Spotted by coverity.
>>
>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>> ---
>>   hw/kvm/apic.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
>> index ffe7a52..a0ab503 100644
>> --- a/hw/kvm/apic.c
>> +++ b/hw/kvm/apic.c
>> @@ -29,7 +29,7 @@ void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
>>       APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
>>       int i;
>>
>> -    memset(kapic, 0, sizeof(kapic));
>> +    memset(kapic, 0, sizeof(*kapic));
>>       kvm_apic_set_reg(kapic, 0x2, s->id<<  24);
>>       kvm_apic_set_reg(kapic, 0x8, s->tpr);
>>       kvm_apic_set_reg(kapic, 0xd, s->log_dest<<  24);
>>      
> Yep, that's what I actually meant...
>
> Thanks,
> Jan
>
>    

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Hello Anthony,

this patch should be committed to QEMU 1.1.
I had sent a patch with the same fix 6 days later.

Regards,

Stefan W.

Re: [Qemu-devel] [PATCH 1.1 1/2] kvm/apic: correct short memset

[Jan Kiszka]

On 2012-05-22 17:30, Stefan Weil wrote:
> Am 10.05.2012 19:28, schrieb Jan Kiszka:
>> On 2012-05-10 13:19, Jim Meyering wrote:
>>    
>>> From: Jim Meyering<meyering@redhat.com>
>>>
>>> kvm_put_apic_state's attempt to clear *kapic before setting its
>>> bits cleared sizeof(void*) bytes (no more than 8) rather than the
>>> intended 1024 (KVM_APIC_REG_SIZE) bytes. Spotted by coverity.
>>>
>>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>>> ---
>>>   hw/kvm/apic.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
>>> index ffe7a52..a0ab503 100644
>>> --- a/hw/kvm/apic.c
>>> +++ b/hw/kvm/apic.c
>>> @@ -29,7 +29,7 @@ void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
>>>       APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
>>>       int i;
>>>
>>> -    memset(kapic, 0, sizeof(kapic));
>>> +    memset(kapic, 0, sizeof(*kapic));
>>>       kvm_apic_set_reg(kapic, 0x2, s->id<<  24);
>>>       kvm_apic_set_reg(kapic, 0x8, s->tpr);
>>>       kvm_apic_set_reg(kapic, 0xd, s->log_dest<<  24);
>>>      
>> Yep, that's what I actually meant...
>>
>> Thanks,
>> Jan
>>
>>    
> 
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> 
> Hello Anthony,
> 
> this patch should be committed to QEMU 1.1.
> I had sent a patch with the same fix 6 days later.

Thanks for reminding. Yes, please merge!

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1.1 1/2] kvm/apic: correct short memset

[Stefan Weil]

Am 23.05.2012 12:33, schrieb Jan Kiszka:
> On 2012-05-22 17:30, Stefan Weil wrote:
>> Am 10.05.2012 19:28, schrieb Jan Kiszka:
>>> On 2012-05-10 13:19, Jim Meyering wrote:
>>>
>>>> From: Jim Meyering<meyering@redhat.com>
>>>>
>>>> kvm_put_apic_state's attempt to clear *kapic before setting its
>>>> bits cleared sizeof(void*) bytes (no more than 8) rather than the
>>>> intended 1024 (KVM_APIC_REG_SIZE) bytes. Spotted by coverity.
>>>>
>>>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>>>> ---
>>>>    hw/kvm/apic.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
>>>> index ffe7a52..a0ab503 100644
>>>> --- a/hw/kvm/apic.c
>>>> +++ b/hw/kvm/apic.c
>>>> @@ -29,7 +29,7 @@ void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
>>>>        APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
>>>>        int i;
>>>>
>>>> -    memset(kapic, 0, sizeof(kapic));
>>>> +    memset(kapic, 0, sizeof(*kapic));
>>>>        kvm_apic_set_reg(kapic, 0x2, s->id<<   24);
>>>>        kvm_apic_set_reg(kapic, 0x8, s->tpr);
>>>>        kvm_apic_set_reg(kapic, 0xd, s->log_dest<<   24);
>>>>
>>> Yep, that's what I actually meant...
>>>
>>> Thanks,
>>> Jan
>>>
>>>
>>
>> Reviewed-by: Stefan Weil<sw@weilnetz.de>
>>
>> Hello Anthony,
>>
>> this patch should be committed to QEMU 1.1.
>> I had sent a patch with the same fix 6 days later.
>
> Thanks for reminding. Yes, please merge!
>
> Jan


Ping?

This is one of the bug fixes which is missing in QEMU 1.1,
and it is also missing in latest QEMU git master.

What can be done to get it committed?

Cheers,

Stefan W.

Re: [Qemu-devel] [PATCH 2/2] cadence_gem: avoid stack-writing buffer-overrun

[Stefan Weil]

Am 14.05.2012 06:57, schrieb Peter Crosthwaite:
> ACK and Thanks Jim,
>
> Reviewed-by: Peter A.G. Crosthwaite<peter.crosthwaite@petalogix.com>
>
> On Fri, May 11, 2012 at 2:19 AM, Jim Meyering<jim@meyering.net>  wrote:
>    
>> From: Jim Meyering<meyering@redhat.com>
>>
>> Use sizeof(rxbuf)-size (not sizeof(rxbuf-size)) as the number
>> of bytes to clear.  The latter would always clear 4 or 8
>> bytes, possibly writing beyond the end of that stack buffer.
>> Alternatively, depending on the value of the "size" parameter,
>> it could fail to initialize the end of "rxbuf".
>> Spotted by coverity.
>>
>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>> ---
>>   hw/cadence_gem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/cadence_gem.c b/hw/cadence_gem.c
>> index e2140ae..dbde392 100644
>> --- a/hw/cadence_gem.c
>> +++ b/hw/cadence_gem.c
>> @@ -664,7 +664,7 @@ static ssize_t gem_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
>>           */
>>
>>          memcpy(rxbuf, buf, size);
>> -        memset(rxbuf + size, 0, sizeof(rxbuf - size));
>> +        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
>>          rxbuf_ptr = rxbuf;
>>          crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
>>          if (size<  60) {
>> --
>> 1.7.10.1.487.ga3935e6
>>      


Ping. This patch is still missing in 1.1 and master.

Re: [Qemu-devel] [PATCH 1.1 1/2] kvm/apic: correct short memset

[Jan Kiszka]

On 2012-06-10 22:29, Stefan Weil wrote:
> Am 23.05.2012 12:33, schrieb Jan Kiszka:
>> On 2012-05-22 17:30, Stefan Weil wrote:
>>> Am 10.05.2012 19:28, schrieb Jan Kiszka:
>>>> On 2012-05-10 13:19, Jim Meyering wrote:
>>>>
>>>>> From: Jim Meyering<meyering@redhat.com>
>>>>>
>>>>> kvm_put_apic_state's attempt to clear *kapic before setting its
>>>>> bits cleared sizeof(void*) bytes (no more than 8) rather than the
>>>>> intended 1024 (KVM_APIC_REG_SIZE) bytes. Spotted by coverity.
>>>>>
>>>>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>>>>> ---
>>>>>    hw/kvm/apic.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/kvm/apic.c b/hw/kvm/apic.c
>>>>> index ffe7a52..a0ab503 100644
>>>>> --- a/hw/kvm/apic.c
>>>>> +++ b/hw/kvm/apic.c
>>>>> @@ -29,7 +29,7 @@ void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
>>>>>        APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
>>>>>        int i;
>>>>>
>>>>> -    memset(kapic, 0, sizeof(kapic));
>>>>> +    memset(kapic, 0, sizeof(*kapic));
>>>>>        kvm_apic_set_reg(kapic, 0x2, s->id<<   24);
>>>>>        kvm_apic_set_reg(kapic, 0x8, s->tpr);
>>>>>        kvm_apic_set_reg(kapic, 0xd, s->log_dest<<   24);
>>>>>
>>>> Yep, that's what I actually meant...
>>>>
>>>> Thanks,
>>>> Jan
>>>>
>>>>
>>>
>>> Reviewed-by: Stefan Weil<sw@weilnetz.de>
>>>
>>> Hello Anthony,
>>>
>>> this patch should be committed to QEMU 1.1.
>>> I had sent a patch with the same fix 6 days later.
>>
>> Thanks for reminding. Yes, please merge!
>>
>> Jan
> 
> 
> Ping?
> 
> This is one of the bug fixes which is missing in QEMU 1.1,
> and it is also missing in latest QEMU git master.
> 
> What can be done to get it committed?

Avi or Marcelo, please queue in uq/master and send a pull soon!

That reminds me that [1] is still awaiting comments (and further
testing). Critical for qemu-kvm 1.1 and qemu 1.1.1 as well.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/92036

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/2] kvm/apic: correct short memset

[Avi Kivity]

On 05/10/2012 07:19 PM, Jim Meyering wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> kvm_put_apic_state's attempt to clear *kapic before setting its
> bits cleared sizeof(void*) bytes (no more than 8) rather than the
> intended 1024 (KVM_APIC_REG_SIZE) bytes. Spotted by coverity.

Thanks, applied to uq/master.


-- 
error compiling committee.c: too many arguments to function