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.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
