[Skiboot] [PATCH v8 07/11] Load the ultravisor from flash and decompress

[Oliver O'Halloran]

On Wed, Sep 9, 2020 at 6:19 AM Ryan Grimm <grimm at linux.vnet.ibm.com> wrote:
>
> On Fri, 2020-08-28 at 14:54 +1000, Oliver O'Halloran wrote:
> >
> > > +static int uv_decompress_image(void)
> > > +{
> > > +       struct xz_decompress uv_xz;
> > > +       uint64_t uv_fw_size;
> > > +
> > > +       if (!uv_image) {
> > > +               prerror("UV: Preload hasn't started yet!
> > > Aborting.\n");
> > > +               return OPAL_INTERNAL_ERROR;
> > > +       }
> > > +
> > > +       if (wait_for_resource_loaded(RESOURCE_ID_UV_IMAGE,
> > > +                                    RESOURCE_SUBID_NONE) !=
> > > OPAL_SUCCESS) {
> > > +               prerror("UV: Ultravisor image load failed\n");
> > > +               return OPAL_INTERNAL_ERROR;
> > > +       }
> > > +
> > > +       uv_xz.dst = (void *)dt_get_address(uv_fw_node, 0,
> > > &uv_fw_size);
> > > +       uv_xz.dst_size = uv_fw_size;
> > > +       uv_xz.src_size = uv_image_size;
> > > +       uv_xz.src = uv_image;
> > > +
> > > +       if (stb_is_container((void*)uv_xz.src, uv_xz.src_size))
> > > +               uv_xz.src = uv_xz.src + SECURE_BOOT_HEADERS_SIZE;
> > > +
> > > +       xz_start_decompress(&uv_xz);
> > > +               if ((uv_xz.status != OPAL_PARTIAL) && (uv_xz.status
> > > != OPAL_SUCCESS)) {
> >
> > Weird indentation and why would we get OPAL_PARTIAL here?
> >
>
> Oh, oops, indents fixed.
>
> Well, the comment in xz_start_decompress show this:
>
>  * The `status` value will be OPAL_PARTIAL till the job completes
> (successfully
>  * or not)
>
> The code seems to work that way and it's a job queue so we need to
> check for both since it might not complete right?

On closer inspection this whole thing is totally broken. uv_xz is
allocated on the stack then passed to a worker via cpu_queue_job().
Immediately afterwards we return from the function so uv_xz goes out
of scope and the worker is looking at stack garbage. On top of that
uv_image is freed soon afterwards once we've returned to  init_uv().
There's also the problem of the job API not actually guaranteeing that
any part of the worker has executed, so even if we ignore the above
checking uv_xz.status at this point is going to be broken since the
worker may not have written anything to it yet.

At a guess there's supposed to be a call to wait_xz_decompressed()
before checking the result in uv_xz. That would probably fix all those
problems.

Oliver