[Skiboot] [PATCH v8 07/11] Load the ultravisor from flash and decompress
Oliver O'Halloran
oohall at gmail.com
Wed Sep 9 12:51:29 AEST 2020
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
More information about the Skiboot
mailing list