Re: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper

[Ming Lei]

On Wed, Jun 1, 2016 at 11:35 PM,  <Mario_Limonciello@xxxxxxxx> wrote:
>> -----Original Message-----
>> From: Limonciello, Mario
>> Sent: Monday, May 16, 2016 1:28 PM
>> To: ming.lei@xxxxxxxxxxxxx
>> Cc: LKML <linux-kernel@xxxxxxxxxxxxxxx>; Limonciello, Mario
>> <Mario_Limonciello@xxxxxxxx>
>> Subject: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper
>>
>> when loading firmware dell_rbu previously would allow a userspace
>> application to craft the payload after dell_rbu was loaded and abuse the
>> udev userspace API.
>>
>> Instead require the payload to be crafted and placed in
>> /lib/firmware/dell_rbu ahead of time.
>>
>> This adjusts dell_rbu to immediately load the firmware from
>> /lib/firmware/dell_rbu when "init" is passed into image_type using the
>> kernel helper.
>>
>> Signed-off-by: Mario Limonciello <mario_limonciello@xxxxxxxx>
>> ---
>>  drivers/firmware/Kconfig    | 1 -
>>  drivers/firmware/dell_rbu.c | 2 +-
>>  2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index
>> 6664f11..85afe59 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -86,7 +86,6 @@ config DELL_RBU
>>       tristate "BIOS update support for DELL systems via sysfs"
>>       depends on X86
>>       select FW_LOADER
>> -     select FW_LOADER_USER_HELPER
>>       help
>>        Say m if you want to have the option of updating the BIOS for your
>>        DELL system. Note you need a Dell OpenManage or Dell Update
>> package (DUP) diff --git a/drivers/firmware/dell_rbu.c
>> b/drivers/firmware/dell_rbu.c index 2f452f1..77b2a77 100644
>> --- a/drivers/firmware/dell_rbu.c
>> +++ b/drivers/firmware/dell_rbu.c
>> @@ -620,7 +620,7 @@ static ssize_t write_rbu_image_type(struct file *filp,
>> struct kobject *kobj,
>>               if (!rbu_data.entry_created) {
>>                       spin_unlock(&rbu_data.lock);
>>                       req_firm_rc =
>> request_firmware_nowait(THIS_MODULE,
>> -                             FW_ACTION_NOHOTPLUG, "dell_rbu",
>> +                             FW_ACTION_HOTPLUG, "dell_rbu",
>>                               &rbu_device->dev, GFP_KERNEL, &context,
>>                               callbackfn_rbu);
>>                       if (req_firm_rc) {
>> --
>> 2.7.4
>
> Hi Ming,
>
> Could you comment on these patches?  There isn't currently a subsystem maintainer for dell_rbu, so I think you are the most qualified to pull these in.  They have the best implications for you since you can drop all that fallback code now too.

Hi Mario,

I am fine for the 1st two patches.

But IMO it isn't good to drop support for
FW_LOADER_USER_HELPER_FALLBACK inside kernel now because udev
isn't used everywhere, and not every distribution always put the firmware files
under the default builtin path.

Thanks,