Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411341 > unrolled thread

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

Started by<Mario_Limonciello@Dell.com>
First post2016-06-01 17:40 +0200
Last post2016-06-02 04:40 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper <Mario_Limonciello@Dell.com> - 2016-06-01 17:40 +0200
    Re: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper Ming Lei <ming.lei@canonical.com> - 2016-06-02 04:30 +0200
      RE: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper <Mario_Limonciello@Dell.com> - 2016-06-02 04:40 +0200

#1411341 — RE: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper

From<Mario_Limonciello@Dell.com>
Date2016-06-01 17:40 +0200
SubjectRE: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper
Message-ID<rFgps-1H0-11@gated-at.bofh.it>
> -----Original Message-----
> From: Limonciello, Mario
> Sent: Monday, May 16, 2016 1:28 PM
> To: ming.lei@canonical.com
> Cc: LKML <linux-kernel@vger.kernel.org>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> 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@dell.com>
> ---
>  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.

[toc] | [next] | [standalone]

#1411772

FromMing Lei <ming.lei@canonical.com>
Date2016-06-02 04:30 +0200
Message-ID<rFqyu-8tG-15@gated-at.bofh.it>
In reply to#1411341
On Wed, Jun 1, 2016 at 11:35 PM,  <Mario_Limonciello@dell.com> wrote:
>> -----Original Message-----
>> From: Limonciello, Mario
>> Sent: Monday, May 16, 2016 1:28 PM
>> To: ming.lei@canonical.com
>> Cc: LKML <linux-kernel@vger.kernel.org>; Limonciello, Mario
>> <Mario_Limonciello@Dell.com>
>> 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@dell.com>
>> ---
>>  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,

[toc] | [prev] | [next] | [standalone]

#1411780

From<Mario_Limonciello@Dell.com>
Date2016-06-02 04:40 +0200
Message-ID<rFqI9-50-13@gated-at.bofh.it>
In reply to#1411772
> -----Original Message-----
> From: Ming Lei [mailto:ming.lei@canonical.com]
> Sent: Wednesday, June 1, 2016 9:24 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v2 1/3] dell_rbu: Don't fallback to userhelper
> 
> On Wed, Jun 1, 2016 at 11:35 PM,  <Mario_Limonciello@dell.com> wrote:
> >> -----Original Message-----
> >> From: Limonciello, Mario
> >> Sent: Monday, May 16, 2016 1:28 PM
> >> To: ming.lei@canonical.com
> >> Cc: LKML <linux-kernel@vger.kernel.org>; Limonciello, Mario
> >> <Mario_Limonciello@Dell.com>
> >> 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@dell.com>
> >> ---
> >>  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,

Ming,

OK that's fine to me.  
I just thought it would simplify your code, didn't realize anything there were distros still putting firmware in odd places or not using udev.
If you could include the first two patches in your tree for next kernel I would appreciate.  

Thanks,

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web