Re: [PATCH RFC 0/4] Add firmware signature file check

[Takashi Iwai]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/