pci: Error on PCI capability collisions
PATCH 1/1 pci: Error on PCI capability collisions
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
--- hw/pci.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)
hw/pci.c +14 -0
@@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { +if (pdev->used[i]) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, pdev->config_map[i], i); +return -EFAULT ; + } + } } config = pdev->config + offset;
Signature · 1 line
1.7.3.4
Quoted 7 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>
I'll stick an assert there instead. Normal devices don't generate overlapping caps unless there's a bug, and device assignment should do it's own checks.
I really have a mind to rip out the used array too.
Quoted 36 lines
--- hw/pci.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)hw/pci.c +14
-0@@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { +if (pdev->used[i]) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, pdev->config_map[i], i); +return -EFAULT ; + } + } } config = pdev->config + offset;Signature · 1 line
1.7.3.4
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Quoted 7 lines
Quoted 1 line
Signed-off-by Jan Kiszka <[email protected]>I'll stick an assert there instead. Normal devices don't generate overlapping caps unless there's a bug, and device assignment should do it's own checks.
I really have a mind to rip out the used array too.
So you'd rather kill qemu rather than have a reasonable error return path... great :(
Alex
Quoted 36 lines
Quoted 36 lines
--- hw/pci.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)hw/pci.c +14
-0@@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { +if (pdev->used[i]) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, pdev->config_map[i], i); +return -EFAULT ; + } + } } config = pdev->config + offset;Signature · 1 line
1.7.3.4
Quoted 14 lines · Alex Williamson
Quoted 13 lines · Michael S. Tsirkin
Quoted 7 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>I'll stick an assert there instead. Normal devices don't generate overlapping caps unless there's a bug, and device assignment should do it's own checks.
Quoted 6 lines
Quoted 1 line
I really have a mind to rip out the used array too.
So you'd rather kill qemu rather than have a reasonable error return path... great :(
Alex
Well that will make it possible to make pci_add_capability return void, less work for callers :) Dev assignment is really the only place where capability offsets need to be verified.
Quoted 38 lines
Quoted 36 lines
Quoted 36 lines
--- hw/pci.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)hw/pci.c +14
-0@@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { +if (pdev->used[i]) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, pdev->config_map[i], i); +return -EFAULT ; + } + } } config = pdev->config + offset;Signature · 1 line
1.7.3.4
Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Alex Williamson
Quoted 14 lines · Michael S. Tsirkin
Quoted 7 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>I'll stick an assert there instead. Normal devices don't generate overlapping caps unless there's a bug, and device assignment should do it's own checks.
I really have a mind to rip out the used array too.
Quoted 8 lines
Quoted 4 lines
So you'd rather kill qemu rather than have a reasonable error return path... great :(
Alex
Well that will make it possible to make
pci_add_capabilityreturn void, less work for callers :) Dev assignment is really the only place where capability offsets need to be verified.
A few issues with that... Since when is error handling so difficult that we need to pretend that nothing ever fails just to make it easy for the caller? Why is device assignment such a special case? It's actually rather ironic that we're trying to add error checking to catch bugs that real hardware is exposing, but assuming that emulated drivers always get it right. How will a return void help the emulated driver that has a coding error?
Alex
Quoted 38 lines
Quoted 38 lines
Quoted 36 lines
Quoted 36 lines
--- hw/pci.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)hw/pci.c +14
-0@@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { +if (pdev->used[i]) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, pdev->config_map[i], i); +return -EFAULT ; + } + } } config = pdev->config + offset;Signature · 1 line
1.7.3.4
Quoted 23 lines · Alex Williamson
Quoted 22 lines · Michael S. Tsirkin
Quoted 20 lines · Alex Williamson
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Quoted 7 lines
Quoted 1 line
Signed-off-by Jan Kiszka <[email protected]>I'll stick an assert there instead. Normal devices don't generate overlapping caps unless there's a bug, and device assignment should do it's own checks.
I really have a mind to rip out the used array too.
So you'd rather kill qemu rather than have a reasonable error return path... great :(
Alex
Quoted 7 lines
Quoted 3 lines
Well that will make it possible to make
pci_add_capabilityreturn void, less work for callers :) Dev assignment is really the only place where capability offsets need to be verified.A few issues with that... Since when is error handling so difficult that we need to pretend that nothing ever fails just to make it easy for the caller?
It isn't but no need to introduce error codes just for fun.
Quoted 1 line
Why is device assignment such a special case?
Assigned devices are under the guest control so should be assumed untrusted, and we must verify anything we get from them.
For example, I think it's generally a mistake to read a device register and use that as an array index, we must check it's in range first. It's best to do these range checks in the dev assignment code so that it's easy to verify that all values are used safely.
Quoted 5 lines
It's actually
rather ironic that we're trying to add error checking to catch bugs that real hardware is exposing, but assuming that emulated drivers always get it right. How will a return void help the emulated driver that has a coding error?
Drivers use fixed offsets so they will always fail or always work. If we return an error they might seem to work but behave incrrectly without the right capability.
Signature · 1 line
MST
Quoted 28 lines · Michael S. Tsirkin
Quoted 27 lines · Alex Williamson
Quoted 25 lines · Michael S. Tsirkin
Quoted 14 lines · Alex Williamson
Quoted 13 lines · Michael S. Tsirkin
Quoted 7 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>I'll stick an assert there instead. Normal devices don't generate overlapping caps unless there's a bug, and device assignment should do it's own checks.
Quoted 6 lines
Quoted 1 line
I really have a mind to rip out the used array too.
So you'd rather kill qemu rather than have a reasonable error return path... great :(
Alex
Well that will make it possible to make
pci_add_capabilityreturn void, less work for callers :) Dev assignment is really the only place where capability offsets need to be verified.
Quoted 5 lines
Quoted 3 lines
A few issues with that... Since when is error handling so difficult that we need to pretend that nothing ever fails just to make it easy for the caller?
It isn't but no need to introduce error codes just for fun.
Likewise no need to remove error handling code because you dislike it.
Quoted 4 lines
Quoted 1 line
Why is device assignment such a special case?
Assigned devices are under the guest control so should be assumed untrusted, and we must verify anything we get from them.
The device is under guest control, but setting up access to the device is entirely under qemu control.
Quoted 4 lines
For example, I think it's generally a mistake to read a device register and use that as an array index, we must check it's in range first. It's best to do these range checks in the dev assignment code so that it's easy to verify that all values are used safely.
Except you're assuming that emulated drivers always get it right and don't need such range checks, which is completely bogus. We can either bloat the device assignment code with wrappers that do range checks for every pci callback or have pci check for everybody and maybe we'll catch a bug or two...
Quoted 7 lines
Quoted 5 lines
It's actually
rather ironic that we're trying to add error checking to catch bugs that real hardware is exposing, but assuming that emulated drivers always get it right. How will a return void help the emulated driver that has a coding error?Drivers use fixed offsets so they will always fail or always work.
Drivers "often" used fixed offsets. I don't see any requirement for this.
Quoted 2 lines
If we return an error they might seem to work but behave incrrectly without the right capability.
And if we return void and blast capabilities over top of each other, that's ok?
Alex
Quoted 36 lines · Michael S. Tsirkin
Quoted 23 lines · Alex Williamson
Quoted 22 lines · Michael S. Tsirkin
Quoted 20 lines · Alex Williamson
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Jan Kiszka
From: Alex Williamson<[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Quoted 7 lines
Quoted 1 line
Signed-off-by Jan Kiszka<[email protected]>I'll stick an assert there instead. Normal devices don't generate overlapping caps unless there's a bug, and device assignment should do it's own checks.
I really have a mind to rip out the used array too.
So you'd rather kill qemu rather than have a reasonable error return path... great :(
Alex
Quoted 7 lines
Quoted 3 lines
Well that will make it possible to make
pci_add_capabilityreturn void, less work for callers :) Dev assignment is really the only place where capability offsets need to be verified.A few issues with that... Since when is error handling so difficult that we need to pretend that nothing ever fails just to make it easy for the caller?
It isn't but no need to introduce error codes just for fun.
Quoted 1 line
Why is device assignment such a special case?
Quoted 8 lines
Assigned devices are under the guest control so should be assumed untrusted, and we must verify anything we get from them.
For example, I think it's generally a mistake to read a device register and use that as an array index, we must check it's in range first. It's best to do these range checks in the dev assignment code so that it's easy to verify that all values are used safely.
So we want to pollute the dev assignment code with knowledge of this array for bounds checking, which you're threatening to remove?
The patch is simple, the return error checking is simple, and when we write error free code, we can remove all error checking.
I found the current array & it's error checking fairly handy when the array was overflowed and it resulted in oddly succeeding/failing sequences doing device assignment (yes, due to bad hardware -- shocking! ;-) ). The error checking quickly pointed out the problem, and made it easy to debug. I would expect code generators would appreciate keeping the array & it's related checking, like overlap & bounds checking, a welcomed addition.
Adding such features in each potentially error-ing caller doesn't reduce the code size, (it'll have to be replicated in several areas), and the return check
is simple & common (and already exists), so removing it will be
more work then augmenting the existing framework.
additionally, that's assuming the coder creates the correct check,
in different variants/locations.
ACK to Jan's patch.
Quoted 10 lines
Quoted 5 lines
It's actually
rather ironic that we're trying to add error checking to catch bugs that real hardware is exposing, but assuming that emulated drivers always get it right. How will a return void help the emulated driver that has a coding error?Drivers use fixed offsets so they will always fail or always work. If we return an error they might seem to work but behave incrrectly without the right capability.
Quoted 2 lines · Don Dutile
So we want to pollute the dev assignment code with knowledge of this array for bounds checking, which you're threatening to remove?
OKay, I'll stop being a mule, and apply that patch.
Signature · 1 line
MST
Quoted 7 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>
This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):
staticuint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
{
uint8_t next, prev, found = 0;
if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
return 0;
for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
prev = next + PCI_CAP_LIST_NEXT)
if (next <= offset && next > found)
found = next;
return found;
}Quoted 36 lines
--- hw/pci.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)hw/pci.c +14
-0@@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { +if (pdev->used[i]) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, pdev->config_map[i], i); +return -EFAULT ; + } + } } config = pdev->config + offset;Signature · 1 line
1.7.3.4
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
Quoted 8 lines
Quoted 3 lines
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
Yeah, sorry, forgot to refresh the commit before posting.
Quoted 8 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
Quoted 13 lines
{
uint8_t next, prev, found = 0;
if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
return 0;
for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
prev = next + PCI_CAP_LIST_NEXT)
if (next <= offset && next > found)
found = next;
return found;
}Sounds useful, will enhance the patch.
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
[ Jan: rebased over qemu, minor formatting ]
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’Yeah, sorry, forgot to refresh the commit before posting.
Happens to me too, sometimes.
Quoted 19 lines
Quoted 19 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset){ uint8_t next, prev, found = 0; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) return 0; for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); prev = next + PCI_CAP_LIST_NEXT) if (next <= offset && next > found) found = next;
Quoted 8 lines
Quoted 2 lines
return found; }Sounds useful, will enhance the patch.
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Oh, if you want to just drop that bit, that's also fine. Up to you.
Signature · 1 line
MST
Quoted 36 lines · Jan Kiszka
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
Quoted 8 lines
Quoted 3 lines
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’Yeah, sorry, forgot to refresh the commit before posting.
Quoted 19 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset){ uint8_t next, prev, found = 0; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) return 0; for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); prev = next + PCI_CAP_LIST_NEXT) if (next <= offset && next > found) found = next;
Quoted 8 lines
Quoted 2 lines
return found; }Sounds useful, will enhance the patch.
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Also, let's add a comment documenting the
reason for this check: device assignment
depends on this check to verify that the device
is not broken.
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 42 lines · Michael S. Tsirkin
Quoted 41 lines · Jan Kiszka
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
Quoted 8 lines
Quoted 3 lines
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’Yeah, sorry, forgot to refresh the commit before posting.
Quoted 8 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)Quoted 13 lines
{ uint8_t next, prev, found = 0; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) return 0; for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); prev = next + PCI_CAP_LIST_NEXT) if (next <= offset && next > found) found = next; return found; }Sounds useful, will enhance the patch.
Quoted 8 lines
Quoted 3 lines
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Also, let's add a comment documenting the
reason for this check: device assignment
depends on this check to verify that the device
is not broken.
Based on the previous discussion, I don't think this is accurate as it will also validate emulated devices.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 52 lines · Jan Kiszka
Quoted 42 lines · Michael S. Tsirkin
Quoted 41 lines · Jan Kiszka
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
Quoted 8 lines
Quoted 3 lines
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’Yeah, sorry, forgot to refresh the commit before posting.
Quoted 8 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)Quoted 13 lines
{ uint8_t next, prev, found = 0; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) return 0; for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); prev = next + PCI_CAP_LIST_NEXT) if (next <= offset && next > found) found = next; return found; }Sounds useful, will enhance the patch.
Quoted 8 lines
Quoted 3 lines
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Also, let's add a comment documenting the
reason for this check: device assignment
depends on this check to verify that the device
is not broken.
Quoted 4 lines
Based on the previous discussion, I don't think this is accurate as it will also validate emulated devices.
Jan
Something like the below is accurate, right?
/* Device assignment 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.
*/
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 56 lines · Michael S. Tsirkin
Quoted 55 lines · Jan Kiszka
Quoted 42 lines · Michael S. Tsirkin
Quoted 41 lines · Jan Kiszka
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
Quoted 8 lines
Quoted 3 lines
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’Yeah, sorry, forgot to refresh the commit before posting.
Quoted 8 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)Quoted 13 lines
{ uint8_t next, prev, found = 0; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) return 0; for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); prev = next + PCI_CAP_LIST_NEXT) if (next <= offset && next > found) found = next; return found; }Sounds useful, will enhance the patch.
Quoted 8 lines
Quoted 3 lines
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Also, let's add a comment documenting the
reason for this check: device assignment
depends on this check to verify that the device
is not broken.Based on the previous discussion, I don't think this is accurate as it will also validate emulated devices.
Quoted 8 lines
Quoted 1 line
Jan
Something like the below is accurate, right?
/* Device assignment 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.
*/
I've expressed this in the commit message. Unless there is another reason to do v3, maybe you can merge the comment on commit.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 66 lines · Jan Kiszka
Quoted 56 lines · Michael S. Tsirkin
Quoted 55 lines · Jan Kiszka
Quoted 42 lines · Michael S. Tsirkin
Quoted 41 lines · Jan Kiszka
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
Quoted 8 lines
Quoted 3 lines
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’Yeah, sorry, forgot to refresh the commit before posting.
Quoted 8 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)Quoted 13 lines
{ uint8_t next, prev, found = 0; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) return 0; for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); prev = next + PCI_CAP_LIST_NEXT) if (next <= offset && next > found) found = next; return found; }Sounds useful, will enhance the patch.
Quoted 8 lines
Quoted 3 lines
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Also, let's add a comment documenting the
reason for this check: device assignment
depends on this check to verify that the device
is not broken.Based on the previous discussion, I don't think this is accurate as it will also validate emulated devices.
Quoted 8 lines
Quoted 1 line
Jan
Something like the below is accurate, right?
/* Device assignment 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.
*/
Quoted 4 lines
I've expressed this in the commit message. Unless there is another reason to do v3, maybe you can merge the comment on commit.
Jan
Sure, I can do that, no need with v3. You are fine with the way it's formulated?
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 67 lines · Michael S. Tsirkin
Quoted 66 lines · Jan Kiszka
Quoted 64 lines · Michael S. Tsirkin
Quoted 52 lines · Jan Kiszka
Quoted 42 lines · Michael S. Tsirkin
Quoted 41 lines · Jan Kiszka
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Jan Kiszka
From: Alex Williamson <[email protected]>
Nothing good can happen when we overlap capabilities
Quoted 8 lines
Quoted 3 lines
[ Jan: rebased over qemu, minor formatting ]
Signed-off-by Jan Kiszka <[email protected]>This doesn't build for me:
/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’Yeah, sorry, forgot to refresh the commit before posting.
Quoted 8 lines
I think that what that includes is the capability including each given offset, right? It would be easy to write some code scanning the capability list to figure this value out.
Something along the lines of (untested):static
uint8_tpci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)Quoted 13 lines
{ uint8_t next, prev, found = 0; if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) return 0; for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); prev = next + PCI_CAP_LIST_NEXT) if (next <= offset && next > found) found = next; return found; }Sounds useful, will enhance the patch.
Quoted 8 lines
Quoted 3 lines
(Originally, I just wanted to reduce the qemu-kvm delta... :) )
Jan
Also, let's add a comment documenting the
reason for this check: device assignment
depends on this check to verify that the device
is not broken.Quoted 4 lines
Based on the previous discussion, I don't think this is accurate as it will also validate emulated devices.
Jan
Something like the below is accurate, right?
/* Device assignment 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.
*/
Quoted 7 lines
Quoted 4 lines
I've expressed this in the commit message. Unless there is another reason to do v3, maybe you can merge the comment on commit.
Jan
Sure, I can do that, no need with v3. You are fine with the way it's formulated?
Yep.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
PATCH v2 1/1 pci: Error on PCI capability collisions
In reply to Jan KiszkaNothing good can happen when we overlap capabilities. This may happen when plugging in assigned devices or when devices models contain bugs. Detect the overlap and report it.
Based on qemu-kvm commit by Alex Williamson.
--- hw/pci.c | 34 ++++++++++++++++++++++++++++++++++ 1 files changed, 34 insertions(+), 0 deletions(-)
Changes in v2:
- properly find the conflicting CAP
- adjust error code
hw/pci.c +34 -0
@@ -1811,6 +1811,25 @@ static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,return next; } +static uint8_t pci_find_capability_at_offset (PCIDevice *pdev,uint8_t offset) +{ +uint8_t next, prev, found =0 ; + +if (!(pdev->used[offset])) { +return 0 ; + } + +assert (pdev->config[PCI_STATUS ] &PCI_STATUS_CAP_LIST ); + +for (prev =PCI_CAPABILITY_LIST ; (next = pdev->config[prev]); + prev = next +PCI_CAP_LIST_NEXT ) { +if (next <= offset && next > found) { + found = next; + } + } +return found; +} +/* Patch the PCI vendor and device ids in a PCI rom image if necessary. This is needed for an option rom which is used for more than one device. */ static void pci_patch_ids (PCIDevice *pdev,uint8_t *ptr,int size)
@@ -1952,11 +1971,26 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i, overlapping_cap; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { + overlapping_cap =pci_find_capability_at_offset (pdev, i); +if (overlapping_cap) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, overlapping_cap, i); +return -EINVAL ; + } + } } config = pdev->config + offset;
Signature · 1 line
1.7.3.4
Michael S. Tsirkin
AppliedQuoted 7 lines · Jan Kiszka
Nothing good can happen when we overlap capabilities. This may happen when plugging in assigned devices or when devices models contain bugs. Detect the overlap and report it.
Based on qemu-kvm commit by Alex Williamson.
Signed-off-by Jan Kiszka <[email protected]>
Applied, thanks everyone.
Quoted 67 lines
--- hw/pci.c | 34 ++++++++++++++++++++++++++++++++++ 1 files changed, 34 insertions(+), 0 deletions(-)Changes in v2:
- properly find the conflicting CAP
- adjust error codehw/pci.c +34
-0@@ -1811,6 +1811,25 @@ static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,return next; } +static uint8_t pci_find_capability_at_offset (PCIDevice *pdev,uint8_t offset) +{ +uint8_t next, prev, found =0 ; + +if (!(pdev->used[offset])) { +return 0 ; + } + +assert (pdev->config[PCI_STATUS ] &PCI_STATUS_CAP_LIST ); + +for (prev =PCI_CAPABILITY_LIST ; (next = pdev->config[prev]); + prev = next +PCI_CAP_LIST_NEXT ) { +if (next <= offset && next > found) { + found = next; + } + } +return found; +} +/* Patch the PCI vendor and device ids in a PCI rom image if necessary. This is needed for an option rom which is used for more than one device. */ static void pci_patch_ids (PCIDevice *pdev,uint8_t *ptr,int size)@@ -1952,11 +1971,26 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { uint8_t *config; +int i, overlapping_cap; +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } + }else { +for (i = offset; i < offset + size; i++) { + overlapping_cap =pci_find_capability_at_offset (pdev, i); +if (overlapping_cap) { +fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " +"Attempt to add PCI capability %x at offset " +"%x overlaps existing capability %x at offset %x\n" , +pci_find_domain (pdev->bus),pci_bus_num (pdev->bus), +PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), + cap_id, offset, overlapping_cap, i); +return -EINVAL ; + } + } } config = pdev->config + offset;Signature · 1 line
1.7.3.4