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

[Paolo Bonzini <pbonzini@redhat.com>]

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