On 13.01.2012, at 02:44, David Gibson wrote: > On Fri, Jan 13, 2012 at 03:23:37AM +0200, Michael S. Tsirkin wrote: >> On Fri, Jan 13, 2012 at 11:26:12AM +1100, David Gibson wrote: >>> On Thu, Jan 12, 2012 at 03:32:32PM +0200, Michael S. Tsirkin wrote: >>>> On Thu, Jan 12, 2012 at 04:46:22PM +1100, David Gibson wrote: >>>>> The pci_host_config_{read,write}_common() functions perform PCI config >>>>> accesses. They take a limit parameter which they appear to be supposed >>>>> to bounds check against, however the bounds checking logic, such as it is, >>>>> is completely broken. >>>>> >>>>> Currently, it takes the minimum of the supplied length and the remaining >>>>> space in the region and passes that as the length to the underlying >>>>> config_{read,write} function pointer. This means that accesses which >>>>> partially overrun the region will be silently truncated - which makes >>>>> little sense. >>>> >>>> Why does it make little sense? Makes sense to me. >>> >>> Well, for starters a partial overrun would have to be an unaligned >>> config space access, which is not supported by PCI. The behaviour if >>> you try is undefined on most bridges and unlikely to be a partial >>> read/write (ignoring the low addr bits would be more likely). >> >> Yes, bus level cycles have dword granularity in PCI. >> But look e.g. at our express implementation. >> Config is memory mapped, so we simply map this as memory into guest. > > So? > >> If you do a large read, what happens on real hardware? >> I'm not sure, but it looks possible that it will get split >> and multiple dword transactions generated on the bus. > > Totally irrelevant in this context. The function definition only > allows a maximum of 4 byte transfers (because of the uint32_ts), so > the bus emulation logic would already have to split a long read into > multiple calls to the function. > >> OTOH our implementation passes the read on as is, so >> it can get a multi-dword. >> I'll try to play with some real systems to see what they do. >> >>> Even if a bridge somehow did a partial access, it's still wrong for >>> reads, since the overrunning bits would typically return 0xff not >>> 0x00, and so you'd need to 0xff pad the result. >> >> Or maybe an error needs to be generated. We really don't know, >> it's up to the caller. >> >>> There's just no point doing anything other than simply failing partial >>> overruns. >> >> There's no point in random code churn either. > > Removing an insane semantic os not random code churn. > >>>>> Accesses which entirely overrun the region will *not* >>>>> be blocked (an exploitable bug) >>>>> , because in that case (limit - addr) will >>>>> be negative and so the unsigned MIN will always return len instead. Even >>>>> if signed arithmetic was used, the config_{read,write} callback wouldn't >>>>> know what to do with a negative len parameter. >>>> >>>> The assumption was callers never pass in such values. >>> >>> So, callers are to to treat this function, taking a limit parameter as >>> having semantics of "checks a pointless edge case, but not the obvious >>> type of overrun". You think that's a sensible semantic for a general >>> helper function? Seriously? >> >> I don't mind adding an extra check at this level. But >> the comment would need to be reworded from >> 'fix a bug' to 'pseries wants to pass out of range >> values so let's check this'. > > Oh, FFS, you're just making excuses for not making a simple fix to > stupid code.
ping? Alex