* [Qemu-devel] [PATCH] PCI: add param check for api @ 2015-11-21 7:45 Cao jin 2016-01-11 8:32 ` Michael Tokarev 0 siblings, 1 reply; 6+ messages in thread From: Cao jin @ 2015-11-21 7:45 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-trivial, mst 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] PCI: add param check for api 2015-11-21 7:45 [Qemu-devel] [PATCH] PCI: add param check for api Cao jin @ 2016-01-11 8:32 ` Michael Tokarev 2016-01-11 9:20 ` Paolo Bonzini 2016-01-12 6:39 ` Cao jin 0 siblings, 2 replies; 6+ messages in thread From: Michael Tokarev @ 2016-01-11 8:32 UTC (permalink / raw) To: Cao jin, qemu-devel; +Cc: qemu-trivial, mst 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) { > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] PCI: add param check for api 2016-01-11 8:32 ` Michael Tokarev @ 2016-01-11 9:20 ` Paolo Bonzini 2016-01-12 7:22 ` Cao jin 2016-01-12 6:39 ` Cao jin 1 sibling, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2016-01-11 9:20 UTC (permalink / raw) To: Michael Tokarev, Cao jin, qemu-devel; +Cc: qemu-trivial, mst 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] PCI: add param check for api 2016-01-11 9:20 ` Paolo Bonzini @ 2016-01-12 7:22 ` Cao jin 2016-01-12 12:04 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Cao jin @ 2016-01-12 7:22 UTC (permalink / raw) To: Paolo Bonzini, Michael Tokarev, qemu-devel; +Cc: qemu-trivial, mst 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] PCI: add param check for api 2016-01-12 7:22 ` Cao jin @ 2016-01-12 12:04 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2016-01-12 12:04 UTC (permalink / raw) To: Cao jin, Michael Tokarev, qemu-devel; +Cc: qemu-trivial, mst 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] PCI: add param check for api 2016-01-11 8:32 ` Michael Tokarev 2016-01-11 9:20 ` Paolo Bonzini @ 2016-01-12 6:39 ` Cao jin 1 sibling, 0 replies; 6+ messages in thread From: Cao jin @ 2016-01-12 6:39 UTC (permalink / raw) To: Michael Tokarev, qemu-devel; +Cc: qemu-trivial, mst 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-12 12:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-21 7:45 [Qemu-devel] [PATCH] PCI: add param check for api Cao jin 2016-01-11 8:32 ` Michael Tokarev 2016-01-11 9:20 ` Paolo Bonzini 2016-01-12 7:22 ` Cao jin 2016-01-12 12:04 ` Paolo Bonzini 2016-01-12 6:39 ` Cao jin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).