Re: [Qemu-ppc] [PATCH 3/9] pci: Make bounds checks on config space accesses actually work

[Alexander Graf]

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