Re: [PATCH qemu v2] spapr: Kill SLOF

[David Gibson]

On Wed, Jan 08, 2020 at 03:20:22PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 07/01/2020 16:54, David Gibson wrote:
> > On Tue, Jan 07, 2020 at 03:44:35PM +1100, Alexey Kardashevskiy wrote:
> >>
> >>
> >> On 06/01/2020 15:19, David Gibson wrote:
> >>>> +
> >>>> +static uint32_t client_package_to_path(const void *fdt, uint32_t 
> >>>> phandle,
> >>>> +                                       uint32_t buf, uint32_t len)
> >>>> +{
> >>>> +    char tmp[256];
> >>>
> >>> Fixed sized buffers are icky.  You could either dynamically allocate
> >>> this based on the size the client gives, or you could use
> >>> memory_region_get_ram_ptr() to read the data from the tree directly
> >>> into guest memory.
> >>
> >> @len comes from the guest, I am really not comfortable with allocating
> >> whatever (broken) guest requested. And if I limit @len by 1024 or
> >> similar, then a fixed size buffer will do too, no?
> > 
> > I see your point.  Does this call have a way to report failure?  In
> > that case you could outright fail the call if it requests too long a
> > length.
> 
> It returns length which can be 0 to signal an error.
> 
> but with this particular method the bigger problem is that I cannot know
> in advance the actual path length from fdt_get_path(). I could double
> the size until fdt_get_path() succeeded, just seems overkill here.

fdt_get_path() will return -FDT_ERR_NOSPACE if the path doesn't fit in
the provided buffer.  I think that's enough to fail in the relevant
cases.  You could then use a buffer of size min(client provided size,
fixed max size).

I believe I've thought of trying to implement something that returns
what the path length would be without constructing it, but it turns
out to be essentially impossible given the fdt format and the fact
that we don't allocate inside libfdt (basically we have to use the
partially constructed path buf as state information to know how to
proceed with our scan).

> Property names seem to be limited by 32:
> 
> OF1275:
> ===
> nextprop
> IN:phandle, [string] previous, [address] buf
> OUT:  flag
> 
> Copies the name of the property following previous in the property list
> of the device node identified by phandle into buf, as a null-terminated
> string. Buf is the address of a 32-byte region of memory. If previous is
> zero or a pointer to a null string, copies the name of the device node’s
> first property.
> ===

Yeah... IEEE1275 says that, but I don't think most OF implementations
enforce it.  I'm also pretty sure that limit is broken by device trees
in the wild (I remember investigating this when implementing dtc &
libfdt).

> >> btw how exactly can I use memory_region_get_ram_ptr()?
> >> get_system_memory() returns a root MR which is not RAM, RAM is a
> >> "spapr.ram" sub-MR.
> > 
> > Right, but you know that RAM is always at offset 0 within that root
> > MR. 
> 
> Well, it could potentially be more than just one level down in the MR
> tree, for example we could add NUMA MRs and place actual RAM MR
> under these.

Oh.. yeah, sorry, I think you'd need address_space_translate() or
something like it to locate the right MR first.  Which come to think
of it, validates / truncates the length to, so..

> > That said, it doesn't look like it's that easy to bounds check
> > that pointer, so maybe that's not a good idea after all.
> 
> ok.

.. I think that would work after all.

-- 
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

signature.asc
Description: PGP signature