| Message ID | 20100527054442.GI31807@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On Thu, May 27, 2010 at 02:44:42PM +0900, Isaku Yamahata wrote: > address and config_size are both unsigned. > So check which is bigger before minus operation. > Otherwise the result of minus can be unexpected > big value. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> An this happen in practice? If yes, how? > --- > hw/pci.c | 9 +++++++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 3362842..39a6206 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -988,9 +988,14 @@ uint32_t pci_default_read_config(PCIDevice *d, > uint32_t address, int len) > { > uint32_t val = 0; > + uint32_t config_size = pci_config_size(d); > assert(len == 1 || len == 2 || len == 4); > - len = MIN(len, pci_config_size(d) - address); > - memcpy(&val, d->config + address, len); > + if (address < config_size) { > + len = MIN(len, config_size - address); > + memcpy(&val, d->config + address, len); > + } else { > + val = ~0; > + } > return le32_to_cpu(val); > } > > -- > 1.6.6.1 >
On Thu, May 27, 2010 at 05:13:16PM +0300, Michael S. Tsirkin wrote: > On Thu, May 27, 2010 at 02:44:42PM +0900, Isaku Yamahata wrote: > > address and config_size are both unsigned. > > So check which is bigger before minus operation. > > Otherwise the result of minus can be unexpected > > big value. > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > An this happen in practice? If yes, how? Yes, it happens. When MMCONFIG is enabled, Linux tries to determine the configuration space size of pci host bridget by accessing express extended area without checking pci express capability. See pci_cfg_space_size_ext()/pci_cfg_space_size() in linux/drivers/pci/probes.c If the pci host bridge is not express capable, pci_default_read_config() accesses dev->config beyond its size. > > > --- > > hw/pci.c | 9 +++++++-- > > 1 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index 3362842..39a6206 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -988,9 +988,14 @@ uint32_t pci_default_read_config(PCIDevice *d, > > uint32_t address, int len) > > { > > uint32_t val = 0; > > + uint32_t config_size = pci_config_size(d); > > assert(len == 1 || len == 2 || len == 4); > > - len = MIN(len, pci_config_size(d) - address); > > - memcpy(&val, d->config + address, len); > > + if (address < config_size) { > > + len = MIN(len, config_size - address); > > + memcpy(&val, d->config + address, len); > > + } else { > > + val = ~0; > > + } > > return le32_to_cpu(val); > > } > > > > -- > > 1.6.6.1 > > >
diff --git a/hw/pci.c b/hw/pci.c index 3362842..39a6206 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -988,9 +988,14 @@ uint32_t pci_default_read_config(PCIDevice *d, uint32_t address, int len) { uint32_t val = 0; + uint32_t config_size = pci_config_size(d); assert(len == 1 || len == 2 || len == 4); - len = MIN(len, pci_config_size(d) - address); - memcpy(&val, d->config + address, len); + if (address < config_size) { + len = MIN(len, config_size - address); + memcpy(&val, d->config + address, len); + } else { + val = ~0; + } return le32_to_cpu(val); }
address and config_size are both unsigned. So check which is bigger before minus operation. Otherwise the result of minus can be unexpected big value. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)