Re: [PATCH v3 18/19] firmware: Add request_partial_firmware_into_buf()

Kees Cook
2020-07-29T17:43:47Z

On Wed, Jul 29, 2020 at 08:22:17AM +0200, Takashi Iwai wrote:
> On Wed, 29 Jul 2020 03:17:39 +0200,
> Luis Chamberlain wrote:
> >
> > Long ago Takashi had some points about this strategy breaking
> > compressed file use. Was that considered?
>
> As long as I read the patch, it tries to skip both the compressed and
> the fallback loading when FW_OPT_PARTIAL is set, which is good.
>
> However...
>
> > > @@ -771,18 +805,20 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> > > }
> > >
> > > ret = _request_firmware_prepare(&fw, name, device, buf, size,
> > > - opt_flags);
> > > + offset, opt_flags);
> > > if (ret <= 0) /* error or already assigned */
> > > goto out;
> > >
> > > ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
> > > -#ifdef CONFIG_FW_LOADER_COMPRESS
> > > - if (ret == -ENOENT)
> > > +
> > > + /* Only full reads can support decompression, platform, and sysfs. */
> > > + if (!(opt_flags & FW_OPT_PARTIAL))
> > > + nondirect = true;
> > > +
> > > + if (ret == -ENOENT && nondirect)
> > > ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
> > > fw_decompress_xz);
> > > -#endif
>
> ... by dropping this ifdef, the fw loader would try to access *.xz
> file unnecessarily even if CONFIG_FW_LOADER_COMPRESS is disabled.

Ah, good point. I'd added the -ENOENT fw_decompress_xz, but I take your
point about the needless access. I will switch this back to an #ifdef.

--
Kees Cook