[Qemu-devel] [PATCH 0/2] xen_pt: fix two Coverity defects

[Qemu-devel] [PATCH 0/2] xen_pt: fix two Coverity defects

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>


Gonglei (2):
  xen-pt: fix Negative array index read
  xen-pt: fix Out-of-bounds read

 hw/xen/xen_pt_config_init.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/2] xen-pt: fix Negative array index read

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Coverity spot:
Function xen_pt_bar_offset_to_index() may returns a negative
number (-1) value index, which as an index to array d->io_regions.

Let's directly and simply pass index as an argument to
xen_pt_bar_reg_parse().

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/xen/xen_pt_config_init.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index de9a20f..710fe50 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -360,15 +360,13 @@ static uint64_t xen_pt_get_bar_size(PCIIORegion *r)
 }
 
 static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
-                                         XenPTRegInfo *reg)
+                                         int index)
 {
     PCIDevice *d = &s->dev;
     XenPTRegion *region = NULL;
     PCIIORegion *r;
-    int index = 0;
 
     /* check 64bit BAR */
-    index = xen_pt_bar_offset_to_index(reg->offset);
     if ((0 < index) && (index < PCI_ROM_SLOT)) {
         int type = s->real_device.io_regions[index - 1].type;
 
@@ -422,7 +420,7 @@ static int xen_pt_bar_reg_init(XenPCIPassthroughState *s, XenPTRegInfo *reg,
     }
 
     /* set BAR flag */
-    s->bases[index].bar_flag = xen_pt_bar_reg_parse(s, reg);
+    s->bases[index].bar_flag = xen_pt_bar_reg_parse(s, index);
     if (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED) {
         reg_field = XEN_PT_INVALID_REG;
     }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/2] xen-pt: fix Negative array index read

[Stefano Stabellini]

On Sat, 31 Jan 2015, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Coverity spot:
> Function xen_pt_bar_offset_to_index() may returns a negative
> number (-1) value index, which as an index to array d->io_regions.
> 
> Let's directly and simply pass index as an argument to
> xen_pt_bar_reg_parse().
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/xen/xen_pt_config_init.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index de9a20f..710fe50 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -360,15 +360,13 @@ static uint64_t xen_pt_get_bar_size(PCIIORegion *r)
>  }
>  
>  static XenPTBarFlag xen_pt_bar_reg_parse(XenPCIPassthroughState *s,
> -                                         XenPTRegInfo *reg)
> +                                         int index)
>  {
>      PCIDevice *d = &s->dev;
>      XenPTRegion *region = NULL;
>      PCIIORegion *r;
> -    int index = 0;
>  
>      /* check 64bit BAR */
> -    index = xen_pt_bar_offset_to_index(reg->offset);
>      if ((0 < index) && (index < PCI_ROM_SLOT)) {
>          int type = s->real_device.io_regions[index - 1].type;
>  
> @@ -422,7 +420,7 @@ static int xen_pt_bar_reg_init(XenPCIPassthroughState *s, XenPTRegInfo *reg,
>      }
>  
>      /* set BAR flag */
> -    s->bases[index].bar_flag = xen_pt_bar_reg_parse(s, reg);
> +    s->bases[index].bar_flag = xen_pt_bar_reg_parse(s, index);
>      if (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED) {
>          reg_field = XEN_PT_INVALID_REG;
>      }
> -- 
> 1.7.12.4
> 
> 

[Qemu-devel] [PATCH 2/2] xen-pt: fix Out-of-bounds read

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

The array length of s->real_device.io_regions[] is
"PCI_NUM_REGIONS - 1". Add a check, just make Coverity happy.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/xen/xen_pt_config_init.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 710fe50..3c8b0f1 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -443,6 +443,11 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
         return -1;
     }
 
+    if (index == PCI_ROM_SLOT) {
+        XEN_PT_ERR(&s->dev, "Internal error: Access violation at ROM BAR.\n");
+        return -1;
+    }
+
     /* use fixed-up value from kernel sysfs */
     *value = base_address_with_flags(&s->real_device.io_regions[index]);
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/2] xen-pt: fix Out-of-bounds read

[Stefano Stabellini]

On Sat, 31 Jan 2015, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> The array length of s->real_device.io_regions[] is
> "PCI_NUM_REGIONS - 1". Add a check, just make Coverity happy.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/xen/xen_pt_config_init.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 710fe50..3c8b0f1 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -443,6 +443,11 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>          return -1;
>      }
>  
> +    if (index == PCI_ROM_SLOT) {
> +        XEN_PT_ERR(&s->dev, "Internal error: Access violation at ROM BAR.\n");
> +        return -1;
> +    }

Could you please fix the boundaries of the check just above?
Also please avoid using PCI_ROM_SLOT for the array index check, simply
use PCI_NUM_REGIONS.


>      /* use fixed-up value from kernel sysfs */
>      *value = base_address_with_flags(&s->real_device.io_regions[index]);
>  
> -- 
> 1.7.12.4
> 
> 

Re: [Qemu-devel] [PATCH 2/2] xen-pt: fix Out-of-bounds read

[Gonglei]

On 2015/2/10 14:39, Stefano Stabellini wrote:
> On Sat, 31 Jan 2015, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> The array length of s->real_device.io_regions[] is
>> "PCI_NUM_REGIONS - 1". Add a check, just make Coverity happy.
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  hw/xen/xen_pt_config_init.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
>> index 710fe50..3c8b0f1 100644
>> --- a/hw/xen/xen_pt_config_init.c
>> +++ b/hw/xen/xen_pt_config_init.c
>> @@ -443,6 +443,11 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>>          return -1;
>>      }
>>  
>> +    if (index == PCI_ROM_SLOT) {
>> +        XEN_PT_ERR(&s->dev, "Internal error: Access violation at ROM BAR.\n");
>> +        return -1;
>> +    }
> 
> Could you please fix the boundaries of the check just above?
> Also please avoid using PCI_ROM_SLOT for the array index check, simply
> use PCI_NUM_REGIONS.
> 
You meaning is changing the below check:

if (index < 0 || index >= PCI_NUM_REGIONS - 1) {
        XEN_PT_ERR(&s->dev, "Internal error: Invalid BAR index [%d].\n", index);
        return -1;
    }

Isn't it?

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/2] xen-pt: fix Out-of-bounds read

[Stefano Stabellini]

On Tue, 10 Feb 2015, Gonglei wrote:
> On 2015/2/10 14:39, Stefano Stabellini wrote:
> > On Sat, 31 Jan 2015, arei.gonglei@huawei.com wrote:
> >> From: Gonglei <arei.gonglei@huawei.com>
> >>
> >> The array length of s->real_device.io_regions[] is
> >> "PCI_NUM_REGIONS - 1". Add a check, just make Coverity happy.
> >>
> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> ---
> >>  hw/xen/xen_pt_config_init.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> >> index 710fe50..3c8b0f1 100644
> >> --- a/hw/xen/xen_pt_config_init.c
> >> +++ b/hw/xen/xen_pt_config_init.c
> >> @@ -443,6 +443,11 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> >>          return -1;
> >>      }
> >>  
> >> +    if (index == PCI_ROM_SLOT) {
> >> +        XEN_PT_ERR(&s->dev, "Internal error: Access violation at ROM BAR.\n");
> >> +        return -1;
> >> +    }
> > 
> > Could you please fix the boundaries of the check just above?
> > Also please avoid using PCI_ROM_SLOT for the array index check, simply
> > use PCI_NUM_REGIONS.
> > 
> You meaning is changing the below check:
> 
> if (index < 0 || index >= PCI_NUM_REGIONS - 1) {
>         XEN_PT_ERR(&s->dev, "Internal error: Invalid BAR index [%d].\n", index);
>         return -1;
>     }
> 
> Isn't it?
 
that's right

Re: [Qemu-devel] [PATCH 2/2] xen-pt: fix Out-of-bounds read

[Gonglei]

On 2015/2/10 15:00, Stefano Stabellini wrote:
> On Tue, 10 Feb 2015, Gonglei wrote:
>> On 2015/2/10 14:39, Stefano Stabellini wrote:
>>> On Sat, 31 Jan 2015, arei.gonglei@huawei.com wrote:
>>>> From: Gonglei <arei.gonglei@huawei.com>
>>>>
>>>> The array length of s->real_device.io_regions[] is
>>>> "PCI_NUM_REGIONS - 1". Add a check, just make Coverity happy.
>>>>
>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>> ---
>>>>  hw/xen/xen_pt_config_init.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
>>>> index 710fe50..3c8b0f1 100644
>>>> --- a/hw/xen/xen_pt_config_init.c
>>>> +++ b/hw/xen/xen_pt_config_init.c
>>>> @@ -443,6 +443,11 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>>>>          return -1;
>>>>      }
>>>>  
>>>> +    if (index == PCI_ROM_SLOT) {
>>>> +        XEN_PT_ERR(&s->dev, "Internal error: Access violation at ROM BAR.\n");
>>>> +        return -1;
>>>> +    }
>>>
>>> Could you please fix the boundaries of the check just above?
>>> Also please avoid using PCI_ROM_SLOT for the array index check, simply
>>> use PCI_NUM_REGIONS.
>>>
>> You meaning is changing the below check:
>>
>> if (index < 0 || index >= PCI_NUM_REGIONS - 1) {
>>         XEN_PT_ERR(&s->dev, "Internal error: Invalid BAR index [%d].\n", index);
>>         return -1;
>>     }
>>
>> Isn't it?
>  
> that's right
> 
OK, will do, thanks.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/2] xen_pt: fix two Coverity defects

[Gonglei]

On 2015/1/31 15:27, Gonglei (Arei) wrote:

> From: Gonglei <arei.gonglei@huawei.com>
> 
> 
> Gonglei (2):
>   xen-pt: fix Negative array index read
>   xen-pt: fix Out-of-bounds read
> 
>  hw/xen/xen_pt_config_init.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 

Ping...

Regards,
-Gonglei

Re: [Qemu-devel] [Qemu-trivial] [PATCH 0/2] xen_pt: fix two Coverity defects

[Michael Tokarev]

09.02.2015 05:37, Gonglei wrote:
> On 2015/1/31 15:27, Gonglei (Arei) wrote:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Gonglei (2):
>>   xen-pt: fix Negative array index read
>>   xen-pt: fix Out-of-bounds read
>>
>>  hw/xen/xen_pt_config_init.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> Ping...

I thought you want to change the second patch, did I misunderstand?

Thanks,

/mjt