At Tue, 6 Nov 2012 16:04:50 +0800,
Ming Lei wrote:
>
> On Tue, Nov 6, 2012 at 3:32 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > At Tue, 6 Nov 2012 15:16:43 +0800,
> > Ming Lei wrote:
> >>
> >> On Tue, Nov 6, 2012 at 3:03 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> >> >
> >> > Yeah, it's just uncovered in the patch. As a easy solution, apply the
> >> > patch like below to disallow the udev fw loading when signature check
> >> > is enforced.
> >> >
> >> >
> >> > thanks,
> >> >
> >> > Takashi
> >> >
> >> > ---
> >> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> >> > index 575bc4c..93121c3 100644
> >> > --- a/drivers/base/firmware_class.c
> >> > +++ b/drivers/base/firmware_class.c
> >> > @@ -912,6 +912,13 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
> >> > goto handle_fw;
> >> > }
> >> >
> >> > + /* signature check isn't handled via udev fw loading */
> >> > + if (sig_enforce) {
> >> > + fw_load_abort(fw_priv);
> >> > + direct_load = 1;
> >> > + goto handle_fw;
> >> > + }
> >> > +
> >>
> >> The above might be wrong if the firmware file doesn't exist in default
> >> search paths.
> >
> > Heh, I didn't call it's a perfect patch. It's just an easy solution,
> > as mentioned.
> >
> >> You should skip loading from user space only if
> >> verify_signature() returns false. And the udev loading should be
> >> resorted to if there is no such firmware file in default search paths.
> >
> > ... and the kernel should ask udev again for the corresponding
> > signature.
>
> I mean you can't skip user space loading if there is no firmware file
> in the default search path. And you can do it if verify_signature()
> returns false. So you needn't have to implement the signature for
> user space loading.
Right, and it's intentionally dropped so. For the non-default fw
path, it can be added via proc dynamically or via kconfig statically.
If the firmware is generated via udev, then it doesn't make sense to
check a static signature file.
> > I'm too lazy to implement that just for unknown corner
> > cases, so put the patch like above.
>
> There might be some distributions in which the firmwares aren't stored
> under the default search path, so your change may cause regression
> on these distributions. And, it is a easy change in your patch to make
> the situation working.
A "regression" can't happen in this case because the secure boot is
a completely new stuff :) For normal boot, sig_enforce is false, so
no behavior change here (well my patch still applies the signature
check for direct fw loading, but it won't regress at least).
> Also the default search path in firmware_class.c is from built-in path of
> udev, and distributions may customize their firmware path by udev
> configure option.
Well, the default paths in kernel can be changed to follow that as
well, no?
> > Honestly speaking, I have a feeling that we should rather go for
> > getting rid of udev fw loading. The fw loader code is overly complex
>
> Yes, I have the feeling too, but we need to make sure no regressions
> introduced.
Right. And I guess the exceptional firmware case is better found by
checking udev. But it's a bit off topic from secure boot.
> > just for udev handshaking.
> >
> > Do you know how many firmwares still rely on udev...?
>
> Do you know how many distributions have switched to 3.7-rcX to
> start using direct loading?
Obviously no distro releases using 3.7-rc since it's still rc.
But what's your point?
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
