| Message ID | 20210314232913.2607360-5-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | softmmu: Restrict CPU I/O instructions | expand |
On 210315 0029, Philippe Mathieu-Daudé wrote: > Restrict CPU I/O instructions to architectures providing > I/O bus. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > tests/qtest/fuzz/generic_fuzz.c | 16 ++++++++++------ > tests/qtest/fuzz/qtest_wrappers.c | 4 ++++ > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c > index ee8c17a04c4..3e0089f4a63 100644 > --- a/tests/qtest/fuzz/generic_fuzz.c > +++ b/tests/qtest/fuzz/generic_fuzz.c > @@ -304,6 +304,13 @@ static bool get_io_address(address_range *result, AddressSpace *as, > return cb_info.found; > } > > +static bool get_mmio_address(address_range *result, > + uint8_t index, uint32_t offset) > +{ > + return get_io_address(result, &address_space_memory, index, offset); > +} > + > +#ifdef TARGET_HAS_IOPORT > static bool get_pio_address(address_range *result, > uint8_t index, uint16_t offset) > { > @@ -318,12 +325,6 @@ static bool get_pio_address(address_range *result, > return result->addr <= 0xFFFF ? found : false; > } > > -static bool get_mmio_address(address_range *result, > - uint8_t index, uint32_t offset) > -{ > - return get_io_address(result, &address_space_memory, index, offset); > -} > - > static void op_in(QTestState *s, const unsigned char * data, size_t len) > { > enum Sizes {Byte, Word, Long, end_sizes}; > @@ -395,6 +396,7 @@ static void op_out(QTestState *s, const unsigned char * data, size_t len) > break; > } > } > +#endif /* TARGET_HAS_IOPORT */ > > static void op_read(QTestState *s, const unsigned char * data, size_t len) > { > @@ -626,8 +628,10 @@ static void handle_timeout(int sig) > static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size) > { > void (*ops[]) (QTestState *s, const unsigned char* , size_t) = { > +#ifdef TARGET_HAS_IOPORT > [OP_IN] = op_in, > [OP_OUT] = op_out, I think op_pci_read and op_pci_write would need to be disabled as well (at least the way they are implemented now). > +#endif /* TARGET_HAS_IOPORT */ > [OP_READ] = op_read, > [OP_WRITE] = op_write, > [OP_PCI_READ] = op_pci_read, > diff --git a/tests/qtest/fuzz/qtest_wrappers.c b/tests/qtest/fuzz/qtest_wrappers.c > index 921d1e5ed3a..d56dda9e9b8 100644 > --- a/tests/qtest/fuzz/qtest_wrappers.c > +++ b/tests/qtest/fuzz/qtest_wrappers.c > @@ -24,12 +24,14 @@ static bool serialize = true; > RET_TYPE __wrap_##NAME_AND_ARGS;\ > RET_TYPE __real_##NAME_AND_ARGS; > > +#ifdef TARGET_HAS_IOPORT > WRAP(uint8_t , qtest_inb(QTestState *s, uint16_t addr)) > WRAP(uint16_t , qtest_inw(QTestState *s, uint16_t addr)) > WRAP(uint32_t , qtest_inl(QTestState *s, uint16_t addr)) > WRAP(void , qtest_outb(QTestState *s, uint16_t addr, uint8_t value)) > WRAP(void , qtest_outw(QTestState *s, uint16_t addr, uint16_t value)) > WRAP(void , qtest_outl(QTestState *s, uint16_t addr, uint32_t value)) > +#endif /* TARGET_HAS_IOPORT */ > WRAP(uint8_t , qtest_readb(QTestState *s, uint64_t addr)) > WRAP(uint16_t , qtest_readw(QTestState *s, uint64_t addr)) > WRAP(uint32_t , qtest_readl(QTestState *s, uint64_t addr)) > @@ -50,6 +52,7 @@ WRAP(void, qtest_memset(QTestState *s, uint64_t addr, > uint8_t patt, size_t size)) > > > +#ifdef TARGET_HAS_IOPORT > uint8_t __wrap_qtest_inb(QTestState *s, uint16_t addr) > { > if (!serialize) { > @@ -103,6 +106,7 @@ void __wrap_qtest_outl(QTestState *s, uint16_t addr, uint32_t value) > __real_qtest_outl(s, addr, value); > } > } > +#endif /* TARGET_HAS_IOPORT */ > > uint8_t __wrap_qtest_readb(QTestState *s, uint64_t addr) > { > -- > 2.26.2 >
On 15/03/2021 00.29, Philippe Mathieu-Daudé wrote: > Restrict CPU I/O instructions to architectures providing > I/O bus. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > tests/qtest/fuzz/generic_fuzz.c | 16 ++++++++++------ > tests/qtest/fuzz/qtest_wrappers.c | 4 ++++ > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c > index ee8c17a04c4..3e0089f4a63 100644 > --- a/tests/qtest/fuzz/generic_fuzz.c > +++ b/tests/qtest/fuzz/generic_fuzz.c > @@ -304,6 +304,13 @@ static bool get_io_address(address_range *result, AddressSpace *as, > return cb_info.found; > } > > +static bool get_mmio_address(address_range *result, > + uint8_t index, uint32_t offset) > +{ > + return get_io_address(result, &address_space_memory, index, offset); > +} > + > +#ifdef TARGET_HAS_IOPORT Sorry, but the qtests are generic code, I don't think we should introduce target specific ifdefs here...? Thomas
On 3/15/21 6:14 AM, Thomas Huth wrote: > On 15/03/2021 00.29, Philippe Mathieu-Daudé wrote: >> Restrict CPU I/O instructions to architectures providing >> I/O bus. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> tests/qtest/fuzz/generic_fuzz.c | 16 ++++++++++------ >> tests/qtest/fuzz/qtest_wrappers.c | 4 ++++ >> 2 files changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/tests/qtest/fuzz/generic_fuzz.c >> b/tests/qtest/fuzz/generic_fuzz.c >> index ee8c17a04c4..3e0089f4a63 100644 >> --- a/tests/qtest/fuzz/generic_fuzz.c >> +++ b/tests/qtest/fuzz/generic_fuzz.c >> @@ -304,6 +304,13 @@ static bool get_io_address(address_range *result, >> AddressSpace *as, >> return cb_info.found; >> } >> +static bool get_mmio_address(address_range *result, >> + uint8_t index, uint32_t offset) >> +{ >> + return get_io_address(result, &address_space_memory, index, offset); >> +} >> + >> +#ifdef TARGET_HAS_IOPORT > > Sorry, but the qtests are generic code, I don't think we should > introduce target specific ifdefs here...? My view is if you want to generically access an I/O bus, you need to do it via its address space, not the CPU architecture-specific interface. I.e., if an I/O bus is exposed by the PCI function of a south bridge, if you use the correct PCI AS view you can run your test on any architecture providing a PCI bus, not only X86. So yes you are right, and the current code is abusing it. Yes it is fixable but is it worthwhile? Apparently nobody cared, so probably not worthwhile. Let's disregard this series for now. Regards, Phil.
On 15/03/21 06:14, Thomas Huth wrote: >> diff --git a/tests/qtest/fuzz/generic_fuzz.c >> b/tests/qtest/fuzz/generic_fuzz.c >> index ee8c17a04c4..3e0089f4a63 100644 >> --- a/tests/qtest/fuzz/generic_fuzz.c >> +++ b/tests/qtest/fuzz/generic_fuzz.c >> @@ -304,6 +304,13 @@ static bool get_io_address(address_range *result, >> AddressSpace *as, >> return cb_info.found; >> } >> +static bool get_mmio_address(address_range *result, >> + uint8_t index, uint32_t offset) >> +{ >> + return get_io_address(result, &address_space_memory, index, offset); >> +} >> + >> +#ifdef TARGET_HAS_IOPORT > > Sorry, but the qtests are generic code, I don't think we should > introduce target specific ifdefs here...? FWIW this is not a qtest, it's a separate emulator executable and this file is compiled per-target. That said, your objection does apply to patch 5 since libqos is compiled only once for all targets. Paolo
diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index ee8c17a04c4..3e0089f4a63 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -304,6 +304,13 @@ static bool get_io_address(address_range *result, AddressSpace *as, return cb_info.found; } +static bool get_mmio_address(address_range *result, + uint8_t index, uint32_t offset) +{ + return get_io_address(result, &address_space_memory, index, offset); +} + +#ifdef TARGET_HAS_IOPORT static bool get_pio_address(address_range *result, uint8_t index, uint16_t offset) { @@ -318,12 +325,6 @@ static bool get_pio_address(address_range *result, return result->addr <= 0xFFFF ? found : false; } -static bool get_mmio_address(address_range *result, - uint8_t index, uint32_t offset) -{ - return get_io_address(result, &address_space_memory, index, offset); -} - static void op_in(QTestState *s, const unsigned char * data, size_t len) { enum Sizes {Byte, Word, Long, end_sizes}; @@ -395,6 +396,7 @@ static void op_out(QTestState *s, const unsigned char * data, size_t len) break; } } +#endif /* TARGET_HAS_IOPORT */ static void op_read(QTestState *s, const unsigned char * data, size_t len) { @@ -626,8 +628,10 @@ static void handle_timeout(int sig) static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size) { void (*ops[]) (QTestState *s, const unsigned char* , size_t) = { +#ifdef TARGET_HAS_IOPORT [OP_IN] = op_in, [OP_OUT] = op_out, +#endif /* TARGET_HAS_IOPORT */ [OP_READ] = op_read, [OP_WRITE] = op_write, [OP_PCI_READ] = op_pci_read, diff --git a/tests/qtest/fuzz/qtest_wrappers.c b/tests/qtest/fuzz/qtest_wrappers.c index 921d1e5ed3a..d56dda9e9b8 100644 --- a/tests/qtest/fuzz/qtest_wrappers.c +++ b/tests/qtest/fuzz/qtest_wrappers.c @@ -24,12 +24,14 @@ static bool serialize = true; RET_TYPE __wrap_##NAME_AND_ARGS;\ RET_TYPE __real_##NAME_AND_ARGS; +#ifdef TARGET_HAS_IOPORT WRAP(uint8_t , qtest_inb(QTestState *s, uint16_t addr)) WRAP(uint16_t , qtest_inw(QTestState *s, uint16_t addr)) WRAP(uint32_t , qtest_inl(QTestState *s, uint16_t addr)) WRAP(void , qtest_outb(QTestState *s, uint16_t addr, uint8_t value)) WRAP(void , qtest_outw(QTestState *s, uint16_t addr, uint16_t value)) WRAP(void , qtest_outl(QTestState *s, uint16_t addr, uint32_t value)) +#endif /* TARGET_HAS_IOPORT */ WRAP(uint8_t , qtest_readb(QTestState *s, uint64_t addr)) WRAP(uint16_t , qtest_readw(QTestState *s, uint64_t addr)) WRAP(uint32_t , qtest_readl(QTestState *s, uint64_t addr)) @@ -50,6 +52,7 @@ WRAP(void, qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size)) +#ifdef TARGET_HAS_IOPORT uint8_t __wrap_qtest_inb(QTestState *s, uint16_t addr) { if (!serialize) { @@ -103,6 +106,7 @@ void __wrap_qtest_outl(QTestState *s, uint16_t addr, uint32_t value) __real_qtest_outl(s, addr, value); } } +#endif /* TARGET_HAS_IOPORT */ uint8_t __wrap_qtest_readb(QTestState *s, uint64_t addr) {
Restrict CPU I/O instructions to architectures providing I/O bus. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- tests/qtest/fuzz/generic_fuzz.c | 16 ++++++++++------ tests/qtest/fuzz/qtest_wrappers.c | 4 ++++ 2 files changed, 14 insertions(+), 6 deletions(-)