[Qemu-devel] [PATCH] PCI: add param check for api

[Qemu-devel] [PATCH] PCI: add param check for api

[Cao jin]

add param check for pci_add_capability2, as it is a public API.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 168b9cc..6938f64 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2144,6 +2144,9 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
     uint8_t *config;
     int i, overlapping_cap;
 
+    assert(size > 0);
+    assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset);
+
     if (!offset) {
         offset = pci_find_space(pdev, size);
         if (!offset) {
-- 
2.1.0

Re: [Qemu-devel] [PATCH] PCI: add param check for api

[Michael Tokarev]

21.11.2015 10:45, Cao jin wrote:
> add param check for pci_add_capability2, as it is a public API.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 168b9cc..6938f64 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2144,6 +2144,9 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
>      uint8_t *config;
>      int i, overlapping_cap;
>  
> +    assert(size > 0);
> +    assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset);
> +

I'd like to see some ACKs/Reviews for this one, in particular why
size should be != 0.  Also either move offset assert to the below
"else" clause or rewrite it to be offset == 0 instead if !offset :)

Thanks,

/mjt

>      if (!offset) {
>          offset = pci_find_space(pdev, size);
>          if (!offset) {
> 

Re: [Qemu-devel] [PATCH] PCI: add param check for api

[Paolo Bonzini]

On 11/01/2016 09:32, Michael Tokarev wrote:
>> >  
>> > +    assert(size > 0);
>> > +    assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset);
>> > +
> I'd like to see some ACKs/Reviews for this one, in particular why
> size should be != 0.

In fact it should be >= 2, because two bytes are always written below:

    config = pdev->config + offset;
    config[PCI_CAP_LIST_ID] = cap_id;
    config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];

> Also either move offset assert to the below
> "else" clause or rewrite it to be offset == 0 instead if !offset :)

Good idea to move it below, or even to add

    assert(offset >= PCI_CONFIG_HEADER_SIZE);

after the "if", before the "config" assignment.

Paolo

Re: [Qemu-devel] [PATCH] PCI: add param check for api

[Cao jin]

On 01/11/2016 04:32 PM, Michael Tokarev wrote:
> 21.11.2015 10:45, Cao jin wrote:
>> add param check for pci_add_capability2, as it is a public API.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pci.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 168b9cc..6938f64 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2144,6 +2144,9 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
>>       uint8_t *config;
>>       int i, overlapping_cap;
>>
>> +    assert(size > 0);
>> +    assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset);
>> +
>
> I'd like to see some ACKs/Reviews for this one, in particular why
> size should be != 0.

see pci_find_space(), if size == 0, I guess it will always return 0.
But I should admit that I did made a mistake, I will talk about it in
next mail.

Also either move offset assert to the below
> "else" clause or rewrite it to be offset == 0 instead if !offset :)
>
> Thanks,
>
> /mjt
>
>>       if (!offset) {
>>           offset = pci_find_space(pdev, size);
>>           if (!offset) {
>>
>
>
>
> .
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH] PCI: add param check for api

[Cao jin]

Thanks for your time. I almost forget this one...

On 01/11/2016 05:20 PM, Paolo Bonzini wrote:
>
>
> On 11/01/2016 09:32, Michael Tokarev wrote:
>>>>
>>>> +    assert(size > 0);
>>>> +    assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset);
>>>> +
>> I'd like to see some ACKs/Reviews for this one, in particular why
>> size should be != 0.
>
> In fact it should be >= 2, because two bytes are always written below:
>
>      config = pdev->config + offset;
>      config[PCI_CAP_LIST_ID] = cap_id;
>      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
>
>> Also either move offset assert to the below
>> "else" clause or rewrite it to be offset == 0 instead if !offset :)
>
> Good idea to move it below, or even to add
>
>      assert(offset >= PCI_CONFIG_HEADER_SIZE);
>
> after the "if", before the "config" assignment.
>
> Paolo
>
>

Seems I missed that offset == 0 will lead to find a suitable space in 
pci_find_space, and ensure offset >= PCI_CONFIG_HEADER_SIZE. sorry for 
the carelessness mistake.

According to the spec(PCI local spec, chapter 6.3), capability structure 
should be at DWORD boundary and DWORD aligned, so in both 
condition(if...else...), it should follow the spec

if offset == 0, with following line[*], seems it is ok with align issue.

[*] memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));

The else-branch should ensure these too.

Another little question, shouldn`t we check size at first by:

    assert((size % 4) && (size > 0))  ?

I think if caller ensure the effective param maybe it is easier to read, 
so how about following:

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 168b9cc..47cb509 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2144,6 +2144,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t 
cap_id,
      uint8_t *config;
      int i, overlapping_cap;

+    assert(!(size % 4) && (size > 0));
+
      if (!offset) {
          offset = pci_find_space(pdev, size);
          if (!offset) {
@@ -2155,6 +2157,7 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t 
cap_id,
           * depends on this check to verify that the device is not broken.
           * Should never trigger for emulated devices, but it's helpful
           * for debugging these. */
+        assert(!(offset % 4));
          for (i = offset; i < offset + size; i++) {
              overlapping_cap = pci_find_capability_at_offset(pdev, i);
              if (overlapping_cap) {
@@ -2174,7 +2177,7 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t 
cap_id,
      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
      pdev->config[PCI_CAPABILITY_LIST] = offset;
      pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
-    memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));
+    memset(pdev->used + offset, 0xFF, size);
      /* Make capability read-only by default */
      memset(pdev->wmask + offset, 0, size);
      /* Check capability by default */

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH] PCI: add param check for api

[Paolo Bonzini]

On 12/01/2016 08:22, Cao jin wrote:
> Thanks for your time. I almost forget this one...
> 
> On 01/11/2016 05:20 PM, Paolo Bonzini wrote:
>>
>>
>> On 11/01/2016 09:32, Michael Tokarev wrote:
>>>>>
>>>>> +    assert(size > 0);
>>>>> +    assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset);
>>>>> +
>>> I'd like to see some ACKs/Reviews for this one, in particular why
>>> size should be != 0.
>>
>> In fact it should be >= 2, because two bytes are always written below:
>>
>>      config = pdev->config + offset;
>>      config[PCI_CAP_LIST_ID] = cap_id;
>>      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
>>
>>> Also either move offset assert to the below
>>> "else" clause or rewrite it to be offset == 0 instead if !offset :)
>>
>> Good idea to move it below, or even to add
>>
>>      assert(offset >= PCI_CONFIG_HEADER_SIZE);
>>
>> after the "if", before the "config" assignment.
>>
>> Paolo
>>
>>
> 
> Seems I missed that offset == 0 will lead to find a suitable space in
> pci_find_space, and ensure offset >= PCI_CONFIG_HEADER_SIZE. sorry for
> the carelessness mistake.
> 
> According to the spec(PCI local spec, chapter 6.3), capability structure
> should be at DWORD boundary and DWORD aligned, so in both
> condition(if...else...), it should follow the spec
> 
> if offset == 0, with following line[*], seems it is ok with align issue.
> 
> [*] memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));
> 
> The else-branch should ensure these too.
> 
> Another little question, shouldn`t we check size at first by:
> 
>    assert((size % 4) && (size > 0))  ?
> 
> I think if caller ensure the effective param maybe it is easier to read,
> so how about following:
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 168b9cc..47cb509 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2144,6 +2144,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t
> cap_id,
>      uint8_t *config;
>      int i, overlapping_cap;
> 
> +    assert(!(size % 4) && (size > 0));
> +
>      if (!offset) {
>          offset = pci_find_space(pdev, size);
>          if (!offset) {
> @@ -2155,6 +2157,7 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t
> cap_id,
>           * depends on this check to verify that the device is not broken.
>           * Should never trigger for emulated devices, but it's helpful
>           * for debugging these. */
> +        assert(!(offset % 4));
>          for (i = offset; i < offset + size; i++) {
>              overlapping_cap = pci_find_capability_at_offset(pdev, i);
>              if (overlapping_cap) {
> @@ -2174,7 +2177,7 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t
> cap_id,
>      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
>      pdev->config[PCI_CAPABILITY_LIST] = offset;
>      pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
> -    memset(pdev->used + offset, 0xFF, QEMU_ALIGN_UP(size, 4));
> +    memset(pdev->used + offset, 0xFF, size);
>      /* Make capability read-only by default */
>      memset(pdev->wmask + offset, 0, size);
>      /* Check capability by default */

I don't know; I would have to check all calls to pci_add_capability2.
The other patch you had instead was easier to review.  I would start
with a simpler patch that adds "assert(size >= 2)" and "assert(offset >=
PCI_CONFIG_HEADER_SIZE)"; that's the bare minimum needed to avoid
messing up the capability list.

Paolo