* [PATCH v2 3/5] firmware: update usermode helper docs and add SmPL report [not found] <1466117661-22075-1-git-send-email-mcgrof@kernel.org> @ 2016-06-16 22:54 ` Luis R. Rodriguez 2016-06-16 22:54 ` [PATCH v2 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation Luis R. Rodriguez [not found] ` <1471999507-913-1-git-send-email-mcgrof@kernel.org> 2 siblings, 0 replies; 6+ messages in thread From: Luis R. Rodriguez @ 2016-06-16 22:54 UTC (permalink / raw) To: ming.lei, akpm, mmarek, gregkh Cc: linux-kernel, markivx, stephen.boyd, zohar, broonie, tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix, teg, dhowells, Luis R. Rodriguez, linux-leds We've determined that very sadly we cannot nuke the usermode helper. The firmware code is a bit hard to follow, and so is the history, document the reasons for why we cannot remove the usermode helper and the only thing we can do is compartamentalize it. While it, add a Coccinelle SmPL patch to help maintainers police for *infidels* still using the usermode helper. Its accepted that perhaps we can't get rid of all use cases, as otherwise we'd break userspace, so best we can do is go on a hunt and fix incorrect users of it. Drivers can only be transitioned out of the usermode helper once we know old userspace cannot be not be broken by a kernel change. The current SmPL patch reports: $ export COCCI=scripts/coccinelle/api/request_firmware-usermode.cocci $ make coccicheck MODE=report drivers/leds/leds-lp55xx-common.c:227:8-31: WARNING: please check if user really needs the usermode helper drivers/firmware/dell_rbu.c:622:17-40: WARNING: please check if user really needs the usermode helper Cc: Richard Purdie <rpurdie@rpsys.net> Cc: Jacek Anaszewski <j.anaszewski@samsung.com> Cc: linux-leds@vger.kernel.org Cc: Abhay Salunke <Abhay_Salunke@dell.com> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- Documentation/firmware_class/README | 36 ++++++++++++++++++--- .../coccinelle/api/request_firmware-usermode.cocci | 37 ++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 scripts/coccinelle/api/request_firmware-usermode.cocci diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README index 056d1cb9d365..00604b6d7675 100644 --- a/Documentation/firmware_class/README +++ b/Documentation/firmware_class/README @@ -33,7 +33,7 @@ than 256, user should pass 'firmware_class.path=$CUSTOMIZED_PATH' if firmware_class is built in kernel(the general situation) - 2), userspace: + 2), userspace: (AVOID) - /sys/class/firmware/xxx/{loading,data} appear. - hotplug gets called with a firmware identifier in $FIRMWARE and the usual hotplug environment. @@ -41,14 +41,14 @@ 3), kernel: Discard any previous partial load. - 4), userspace: + 4), userspace: (AVOID) - hotplug: cat appropriate_firmware_image > \ /sys/class/firmware/xxx/data 5), kernel: grows a buffer in PAGE_SIZE increments to hold the image as it comes in. - 6), userspace: + 6), userspace: (AVOID) - hotplug: echo 0 > /sys/class/firmware/xxx/loading 7), kernel: request_firmware() returns and the driver has the firmware @@ -66,8 +66,8 @@ copy_fw_to_device(fw_entry->data, fw_entry->size); release_firmware(fw_entry); - Sample/simple hotplug script: - ============================ + Sample/simple hotplug script: (AVOID) + ========================================== # Both $DEVPATH and $FIRMWARE are already provided in the environment. @@ -114,6 +114,32 @@ If you're a maintainer you can help police this with: $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci $ make coccicheck MODE=report +Usermode helper +=============== + +The firmware API supports a usermode helper that lets userspace fetch and +hand off firmware to a driver through sysfs. While in theory this was a +nice feature it also presented many issues, so in practice it should be +avoided at all costs now. + +Only drivers that have a really good reason for a usermode helper should +use it. This use case must be well documented. You should try really hard +to avoid it, at all costs. + +CONFIG_FW_LOADER_USER_HELPER_FALLBACK: disabled by most distributions now +CONFIG_FW_LOADER_USER_HELPER: still enabled by most disributions + +When CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled request_firmware() +*always* calls the usermode helpers. When CONFIG_FW_LOADER_USER_HELPER is +enabled, only if you specifically ask for it wil the usermode helper be +called. If CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled drivers can +still require the usermode helper by using request_firmware_nowait(). + +To help police for user of the usermode helper you can use: + +$ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci +$ make coccicheck MODE=report + about in-kernel persistence: --------------------------- Under some circumstances, as explained below, it would be interesting to keep diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci new file mode 100644 index 000000000000..94ab95cb7c75 --- /dev/null +++ b/scripts/coccinelle/api/request_firmware-usermode.cocci @@ -0,0 +1,37 @@ +// Avoid the usermode helper at all costs +// +// request_firmware_nowait() API enables explicit request for the +// usermode helper. Chances are high its use is just a copy and +// paste bug. Before you fix the driver be sure to *verify* no +// usermode helper tool exists that would otherwise break if +// we replace the driver to use a non-usermode helper variant. +// +// Confidence: High +// +// Reason for low confidence: +// +// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2. +// +// Options: --include-headers + +virtual report +virtual context + +@ r1 depends on report || context @ +expression mod, name, dev, gfp, drv, cb; +position p; +@@ + +( +*request_firmware_nowait@p(mod, false, name, dev, gfp, drv, cb) +| +*request_firmware_nowait@p(mod, 0, name, dev, gfp, drv, cb) +| +*request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb) +) + +@script:python depends on report@ +p << r1.p; +@@ + +coccilib.report.print_report(p[0], "WARNING: please check if user really needs the usermode helper") -- 2.8.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation [not found] <1466117661-22075-1-git-send-email-mcgrof@kernel.org> 2016-06-16 22:54 ` [PATCH v2 3/5] firmware: update usermode helper docs and add SmPL report Luis R. Rodriguez @ 2016-06-16 22:54 ` Luis R. Rodriguez [not found] ` <1471999507-913-1-git-send-email-mcgrof@kernel.org> 2 siblings, 0 replies; 6+ messages in thread From: Luis R. Rodriguez @ 2016-06-16 22:54 UTC (permalink / raw) To: ming.lei, akpm, mmarek, gregkh Cc: linux-kernel, markivx, stephen.boyd, zohar, broonie, tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix, teg, dhowells, Luis R. Rodriguez, linux-leds We need to ensure no one else adds *anything* that requests the usermode helper without really meaning it, to police it we now have an SmPL script but no formal annotation is present to help us ensure the call has been validated. Add a dummy DECLARE_FW_LOADER_USER() which we can use to annotate validated calls and also clearly outline the documentation over where the required usermode helper is documented. All other future callers can be reported via 0-day by making use of the Coccinelle SmPL patch. Cc: Fengguang Wu <fengguang.wu@intel.com> Cc: Richard Purdie <rpurdie@rpsys.net> Cc: Jacek Anaszewski <j.anaszewski@samsung.com> Cc: linux-leds@vger.kernel.org Cc: Abhay Salunke <Abhay_Salunke@dell.com> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/firmware/dell_rbu.c | 1 + drivers/leds/leds-lp55xx-common.c | 1 + include/linux/firmware.h | 7 +++++++ scripts/coccinelle/api/request_firmware-usermode.cocci | 9 ++++++++- 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c index 2f452f1f7c8a..53b3869c0900 100644 --- a/drivers/firmware/dell_rbu.c +++ b/drivers/firmware/dell_rbu.c @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj, return size; } +DECLARE_FW_LOADER_USER("Documentation/dell_rbu.txt"); static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buffer, loff_t pos, size_t count) diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c index 5377f22ff994..ce1d087979a1 100644 --- a/drivers/leds/leds-lp55xx-common.c +++ b/drivers/leds/leds-lp55xx-common.c @@ -219,6 +219,7 @@ out: release_firmware(chip->fw); } +DECLARE_FW_LOADER_USER("Documentation/leds/leds-lp55xx.txt"); static int lp55xx_request_firmware(struct lp55xx_chip *chip) { const char *name = chip->cl->name; diff --git a/include/linux/firmware.h b/include/linux/firmware.h index bdc24ee92823..791ab8632f02 100644 --- a/include/linux/firmware.h +++ b/include/linux/firmware.h @@ -8,6 +8,13 @@ #define FW_ACTION_NOHOTPLUG 0 #define FW_ACTION_HOTPLUG 1 +/* + * Helper for scripts/coccinelle/api/request_firmware-usermode.cocci + * and so users can also easily search for the respectively needed + * usermode helper. + */ +#define DECLARE_FW_LOADER_USER(__usermode_helper) + struct firmware { size_t size; const u8 *data; diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci index 94ab95cb7c75..1775fd3067e8 100644 --- a/scripts/coccinelle/api/request_firmware-usermode.cocci +++ b/scripts/coccinelle/api/request_firmware-usermode.cocci @@ -17,6 +17,13 @@ virtual report virtual context +@ r0 depends on report || context @ +declarer name DECLARE_FW_LOADER_USER; +expression E; +@@ + +DECLARE_FW_LOADER_USER(E); + @ r1 depends on report || context @ expression mod, name, dev, gfp, drv, cb; position p; @@ -30,7 +37,7 @@ position p; *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb) ) -@script:python depends on report@ +@script:python depends on report && !r0 @ p << r1.p; @@ -- 2.8.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <1471999507-913-1-git-send-email-mcgrof@kernel.org>]
* [PATCH v3 3/5] firmware: update usermode helper docs and add SmPL report [not found] ` <1471999507-913-1-git-send-email-mcgrof@kernel.org> @ 2016-08-24 0:45 ` mcgrof 2016-08-24 0:45 ` [PATCH v3 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation mcgrof [not found] ` <1473208930-6835-1-git-send-email-mcgrof@kernel.org> 2 siblings, 0 replies; 6+ messages in thread From: mcgrof @ 2016-08-24 0:45 UTC (permalink / raw) To: ming.lei, akpm, gregkh Cc: daniel.wagner, mmarek, linux-kernel, markivx, stephen.boyd, zohar, broonie, tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix, teg, dhowells, bjorn.andersson, arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds From: "Luis R. Rodriguez" <mcgrof@kernel.org> We've determined that very sadly we cannot nuke the usermode helper. The firmware code is a bit hard to follow, and so is the history, document the reasons for why we cannot remove the usermode helper and the only thing we can do is compartamentalize it. While it, add a Coccinelle SmPL patch to help maintainers police for *infidels* still using the usermode helper. Its accepted that perhaps we can't get rid of all use cases, as otherwise we'd break userspace, so best we can do is go on a hunt and fix incorrect users of it. Drivers can only be transitioned out of the usermode helper once we know old userspace cannot be not be broken by a kernel change. The current SmPL patch reports: $ export COCCI=scripts/coccinelle/api/request_firmware-usermode.cocci $ make coccicheck MODE=report drivers/leds/leds-lp55xx-common.c:227:8-31: WARNING: please check if user really needs the usermode helper drivers/firmware/dell_rbu.c:622:17-40: WARNING: please check if user really needs the usermode helper Cc: Richard Purdie <rpurdie@rpsys.net> Cc: Jacek Anaszewski <j.anaszewski@samsung.com> Cc: linux-leds@vger.kernel.org Cc: Abhay Salunke <Abhay_Salunke@dell.com> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- Documentation/firmware_class/README | 36 ++++++++++++++++++--- .../coccinelle/api/request_firmware-usermode.cocci | 37 ++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 scripts/coccinelle/api/request_firmware-usermode.cocci diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README index 056d1cb9d365..00604b6d7675 100644 --- a/Documentation/firmware_class/README +++ b/Documentation/firmware_class/README @@ -33,7 +33,7 @@ than 256, user should pass 'firmware_class.path=$CUSTOMIZED_PATH' if firmware_class is built in kernel(the general situation) - 2), userspace: + 2), userspace: (AVOID) - /sys/class/firmware/xxx/{loading,data} appear. - hotplug gets called with a firmware identifier in $FIRMWARE and the usual hotplug environment. @@ -41,14 +41,14 @@ 3), kernel: Discard any previous partial load. - 4), userspace: + 4), userspace: (AVOID) - hotplug: cat appropriate_firmware_image > \ /sys/class/firmware/xxx/data 5), kernel: grows a buffer in PAGE_SIZE increments to hold the image as it comes in. - 6), userspace: + 6), userspace: (AVOID) - hotplug: echo 0 > /sys/class/firmware/xxx/loading 7), kernel: request_firmware() returns and the driver has the firmware @@ -66,8 +66,8 @@ copy_fw_to_device(fw_entry->data, fw_entry->size); release_firmware(fw_entry); - Sample/simple hotplug script: - ============================ + Sample/simple hotplug script: (AVOID) + ========================================== # Both $DEVPATH and $FIRMWARE are already provided in the environment. @@ -114,6 +114,32 @@ If you're a maintainer you can help police this with: $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci $ make coccicheck MODE=report +Usermode helper +=============== + +The firmware API supports a usermode helper that lets userspace fetch and +hand off firmware to a driver through sysfs. While in theory this was a +nice feature it also presented many issues, so in practice it should be +avoided at all costs now. + +Only drivers that have a really good reason for a usermode helper should +use it. This use case must be well documented. You should try really hard +to avoid it, at all costs. + +CONFIG_FW_LOADER_USER_HELPER_FALLBACK: disabled by most distributions now +CONFIG_FW_LOADER_USER_HELPER: still enabled by most disributions + +When CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled request_firmware() +*always* calls the usermode helpers. When CONFIG_FW_LOADER_USER_HELPER is +enabled, only if you specifically ask for it wil the usermode helper be +called. If CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled drivers can +still require the usermode helper by using request_firmware_nowait(). + +To help police for user of the usermode helper you can use: + +$ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci +$ make coccicheck MODE=report + about in-kernel persistence: --------------------------- Under some circumstances, as explained below, it would be interesting to keep diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci new file mode 100644 index 000000000000..94ab95cb7c75 --- /dev/null +++ b/scripts/coccinelle/api/request_firmware-usermode.cocci @@ -0,0 +1,37 @@ +// Avoid the usermode helper at all costs +// +// request_firmware_nowait() API enables explicit request for the +// usermode helper. Chances are high its use is just a copy and +// paste bug. Before you fix the driver be sure to *verify* no +// usermode helper tool exists that would otherwise break if +// we replace the driver to use a non-usermode helper variant. +// +// Confidence: High +// +// Reason for low confidence: +// +// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2. +// +// Options: --include-headers + +virtual report +virtual context + +@ r1 depends on report || context @ +expression mod, name, dev, gfp, drv, cb; +position p; +@@ + +( +*request_firmware_nowait@p(mod, false, name, dev, gfp, drv, cb) +| +*request_firmware_nowait@p(mod, 0, name, dev, gfp, drv, cb) +| +*request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb) +) + +@script:python depends on report@ +p << r1.p; +@@ + +coccilib.report.print_report(p[0], "WARNING: please check if user really needs the usermode helper") -- 2.9.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation [not found] ` <1471999507-913-1-git-send-email-mcgrof@kernel.org> 2016-08-24 0:45 ` [PATCH v3 3/5] firmware: update usermode helper docs and add SmPL report mcgrof @ 2016-08-24 0:45 ` mcgrof [not found] ` <1473208930-6835-1-git-send-email-mcgrof@kernel.org> 2 siblings, 0 replies; 6+ messages in thread From: mcgrof @ 2016-08-24 0:45 UTC (permalink / raw) To: ming.lei, akpm, gregkh Cc: daniel.wagner, mmarek, linux-kernel, markivx, stephen.boyd, zohar, broonie, tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix, teg, dhowells, bjorn.andersson, arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds From: "Luis R. Rodriguez" <mcgrof@kernel.org> We need to ensure no one else adds *anything* that requests the usermode helper without really meaning it, to police it we now have an SmPL script but no formal annotation is present to help us ensure the call has been validated. Add a dummy DECLARE_FW_LOADER_USER() which we can use to annotate validated calls and also clearly outline the documentation over where the required usermode helper is documented. All other future callers can be reported via 0-day by making use of the Coccinelle SmPL patch. Cc: Fengguang Wu <fengguang.wu@intel.com> Cc: Richard Purdie <rpurdie@rpsys.net> Cc: Jacek Anaszewski <j.anaszewski@samsung.com> Cc: linux-leds@vger.kernel.org Cc: Abhay Salunke <Abhay_Salunke@dell.com> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/firmware/dell_rbu.c | 1 + drivers/leds/leds-lp55xx-common.c | 1 + include/linux/firmware.h | 7 +++++++ scripts/coccinelle/api/request_firmware-usermode.cocci | 9 ++++++++- 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c index 2f452f1f7c8a..53b3869c0900 100644 --- a/drivers/firmware/dell_rbu.c +++ b/drivers/firmware/dell_rbu.c @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj, return size; } +DECLARE_FW_LOADER_USER("Documentation/dell_rbu.txt"); static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buffer, loff_t pos, size_t count) diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c index 5377f22ff994..ce1d087979a1 100644 --- a/drivers/leds/leds-lp55xx-common.c +++ b/drivers/leds/leds-lp55xx-common.c @@ -219,6 +219,7 @@ out: release_firmware(chip->fw); } +DECLARE_FW_LOADER_USER("Documentation/leds/leds-lp55xx.txt"); static int lp55xx_request_firmware(struct lp55xx_chip *chip) { const char *name = chip->cl->name; diff --git a/include/linux/firmware.h b/include/linux/firmware.h index b1f9f0ccb8ac..147b3062039b 100644 --- a/include/linux/firmware.h +++ b/include/linux/firmware.h @@ -8,6 +8,13 @@ #define FW_ACTION_NOHOTPLUG 0 #define FW_ACTION_HOTPLUG 1 +/* + * Helper for scripts/coccinelle/api/request_firmware-usermode.cocci + * and so users can also easily search for the respectively needed + * usermode helper. + */ +#define DECLARE_FW_LOADER_USER(__usermode_helper) + struct firmware { size_t size; const u8 *data; diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci index 94ab95cb7c75..1775fd3067e8 100644 --- a/scripts/coccinelle/api/request_firmware-usermode.cocci +++ b/scripts/coccinelle/api/request_firmware-usermode.cocci @@ -17,6 +17,13 @@ virtual report virtual context +@ r0 depends on report || context @ +declarer name DECLARE_FW_LOADER_USER; +expression E; +@@ + +DECLARE_FW_LOADER_USER(E); + @ r1 depends on report || context @ expression mod, name, dev, gfp, drv, cb; position p; @@ -30,7 +37,7 @@ position p; *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb) ) -@script:python depends on report@ +@script:python depends on report && !r0 @ p << r1.p; @@ -- 2.9.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <1473208930-6835-1-git-send-email-mcgrof@kernel.org>]
* [PATCH v4 3/5] firmware: update usermode helper docs and add SmPL report [not found] ` <1473208930-6835-1-git-send-email-mcgrof@kernel.org> @ 2016-09-07 0:42 ` Luis R. Rodriguez 2016-09-07 0:42 ` [PATCH v4 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation Luis R. Rodriguez 1 sibling, 0 replies; 6+ messages in thread From: Luis R. Rodriguez @ 2016-09-07 0:42 UTC (permalink / raw) To: ming.lei, akpm, gregkh Cc: daniel.wagner, mmarek, linux-kernel, markivx, stephen.boyd, zohar, broonie, tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix, teg, dhowells, bjorn.andersson, arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds We've determined that very sadly we cannot nuke the usermode helper. The firmware code is a bit hard to follow, and so is the history, document the reasons for why we cannot remove the usermode helper and the only thing we can do is compartamentalize it. While it, add a Coccinelle SmPL patch to help maintainers police for *infidels* still using the usermode helper. Its accepted that perhaps we can't get rid of all use cases, as otherwise we'd break userspace, so best we can do is go on a hunt and fix incorrect users of it. Drivers can only be transitioned out of the usermode helper once we know old userspace cannot be not be broken by a kernel change. The current SmPL patch reports: $ export COCCI=scripts/coccinelle/api/request_firmware-usermode.cocci $ make coccicheck MODE=report drivers/leds/leds-lp55xx-common.c:227:8-31: WARNING: please check if user really needs the usermode helper drivers/firmware/dell_rbu.c:622:17-40: WARNING: please check if user really needs the usermode helper Cc: Richard Purdie <rpurdie@rpsys.net> Cc: Jacek Anaszewski <j.anaszewski@samsung.com> Cc: linux-leds@vger.kernel.org Cc: Abhay Salunke <Abhay_Salunke@dell.com> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- Documentation/firmware_class/README | 36 ++++++++++++++++++--- .../coccinelle/api/request_firmware-usermode.cocci | 37 ++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 scripts/coccinelle/api/request_firmware-usermode.cocci diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README index 20aca5901785..9f8b2daf4c7c 100644 --- a/Documentation/firmware_class/README +++ b/Documentation/firmware_class/README @@ -33,7 +33,7 @@ than 256, user should pass 'firmware_class.path=$CUSTOMIZED_PATH' if firmware_class is built in kernel(the general situation) - 2), userspace: + 2), userspace: (AVOID) - /sys/class/firmware/xxx/{loading,data} appear. - hotplug gets called with a firmware identifier in $FIRMWARE and the usual hotplug environment. @@ -41,14 +41,14 @@ 3), kernel: Discard any previous partial load. - 4), userspace: + 4), userspace: (AVOID) - hotplug: cat appropriate_firmware_image > \ /sys/class/firmware/xxx/data 5), kernel: grows a buffer in PAGE_SIZE increments to hold the image as it comes in. - 6), userspace: + 6), userspace: (AVOID) - hotplug: echo 0 > /sys/class/firmware/xxx/loading 7), kernel: request_firmware() returns and the driver has the firmware @@ -66,8 +66,8 @@ copy_fw_to_device(fw_entry->data, fw_entry->size); release_firmware(fw_entry); - Sample/simple hotplug script: - ============================ + Sample/simple hotplug script: (AVOID) + ========================================== # Both $DEVPATH and $FIRMWARE are already provided in the environment. @@ -116,6 +116,32 @@ If you're a maintainer you can help police this with: $ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci $ make coccicheck MODE=report +Usermode helper +=============== + +The firmware API supports a usermode helper that lets userspace fetch and +hand off firmware to a driver through sysfs. While in theory this was a +nice feature it also presented many issues, so in practice it should be +avoided at all costs now. + +Only drivers that have a really good reason for a usermode helper should +use it. This use case must be well documented. You should try really hard +to avoid it, at all costs. + +CONFIG_FW_LOADER_USER_HELPER_FALLBACK: disabled by most distributions now +CONFIG_FW_LOADER_USER_HELPER: still enabled by most disributions + +When CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled request_firmware() +*always* calls the usermode helpers. When CONFIG_FW_LOADER_USER_HELPER is +enabled, only if you specifically ask for it wil the usermode helper be +called. If CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled drivers can +still require the usermode helper by using request_firmware_nowait(). + +To help police for user of the usermode helper you can use: + +$ export COCCI=scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci +$ make coccicheck MODE=report + about in-kernel persistence: --------------------------- Under some circumstances, as explained below, it would be interesting to keep diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci new file mode 100644 index 000000000000..94ab95cb7c75 --- /dev/null +++ b/scripts/coccinelle/api/request_firmware-usermode.cocci @@ -0,0 +1,37 @@ +// Avoid the usermode helper at all costs +// +// request_firmware_nowait() API enables explicit request for the +// usermode helper. Chances are high its use is just a copy and +// paste bug. Before you fix the driver be sure to *verify* no +// usermode helper tool exists that would otherwise break if +// we replace the driver to use a non-usermode helper variant. +// +// Confidence: High +// +// Reason for low confidence: +// +// Copyright: (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org> GPLv2. +// +// Options: --include-headers + +virtual report +virtual context + +@ r1 depends on report || context @ +expression mod, name, dev, gfp, drv, cb; +position p; +@@ + +( +*request_firmware_nowait@p(mod, false, name, dev, gfp, drv, cb) +| +*request_firmware_nowait@p(mod, 0, name, dev, gfp, drv, cb) +| +*request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb) +) + +@script:python depends on report@ +p << r1.p; +@@ + +coccilib.report.print_report(p[0], "WARNING: please check if user really needs the usermode helper") -- 2.9.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation [not found] ` <1473208930-6835-1-git-send-email-mcgrof@kernel.org> 2016-09-07 0:42 ` [PATCH v4 3/5] firmware: update usermode helper docs and add SmPL report Luis R. Rodriguez @ 2016-09-07 0:42 ` Luis R. Rodriguez 1 sibling, 0 replies; 6+ messages in thread From: Luis R. Rodriguez @ 2016-09-07 0:42 UTC (permalink / raw) To: ming.lei, akpm, gregkh Cc: daniel.wagner, mmarek, linux-kernel, markivx, stephen.boyd, zohar, broonie, tiwai, johannes, chunkeey, hauke, jwboyer, dmitry.torokhov, dwmw2, jslaby, torvalds, luto, fengguang.wu, rpurdie, j.anaszewski, Abhay_Salunke, Julia.Lawall, Gilles.Muller, nicolas.palix, teg, dhowells, bjorn.andersson, arend.vanspriel, kvalo, Luis R. Rodriguez, linux-leds We need to ensure no one else adds *anything* that requests the usermode helper without really meaning it, to police it we now have an SmPL script but no formal annotation is present to help us ensure the call has been validated. Add a dummy DECLARE_FW_LOADER_USER() which we can use to annotate validated calls and also clearly outline the documentation over where the required usermode helper is documented. All other future callers can be reported via 0-day by making use of the Coccinelle SmPL patch. Cc: Fengguang Wu <fengguang.wu@intel.com> Cc: Richard Purdie <rpurdie@rpsys.net> Cc: Jacek Anaszewski <j.anaszewski@samsung.com> Cc: linux-leds@vger.kernel.org Cc: Abhay Salunke <Abhay_Salunke@dell.com> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- drivers/firmware/dell_rbu.c | 1 + drivers/leds/leds-lp55xx-common.c | 1 + include/linux/firmware.h | 7 +++++++ scripts/coccinelle/api/request_firmware-usermode.cocci | 9 ++++++++- 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/dell_rbu.c b/drivers/firmware/dell_rbu.c index 2f452f1f7c8a..53b3869c0900 100644 --- a/drivers/firmware/dell_rbu.c +++ b/drivers/firmware/dell_rbu.c @@ -586,6 +586,7 @@ static ssize_t read_rbu_image_type(struct file *filp, struct kobject *kobj, return size; } +DECLARE_FW_LOADER_USER("Documentation/dell_rbu.txt"); static ssize_t write_rbu_image_type(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buffer, loff_t pos, size_t count) diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c index 5377f22ff994..ce1d087979a1 100644 --- a/drivers/leds/leds-lp55xx-common.c +++ b/drivers/leds/leds-lp55xx-common.c @@ -219,6 +219,7 @@ out: release_firmware(chip->fw); } +DECLARE_FW_LOADER_USER("Documentation/leds/leds-lp55xx.txt"); static int lp55xx_request_firmware(struct lp55xx_chip *chip) { const char *name = chip->cl->name; diff --git a/include/linux/firmware.h b/include/linux/firmware.h index b1f9f0ccb8ac..147b3062039b 100644 --- a/include/linux/firmware.h +++ b/include/linux/firmware.h @@ -8,6 +8,13 @@ #define FW_ACTION_NOHOTPLUG 0 #define FW_ACTION_HOTPLUG 1 +/* + * Helper for scripts/coccinelle/api/request_firmware-usermode.cocci + * and so users can also easily search for the respectively needed + * usermode helper. + */ +#define DECLARE_FW_LOADER_USER(__usermode_helper) + struct firmware { size_t size; const u8 *data; diff --git a/scripts/coccinelle/api/request_firmware-usermode.cocci b/scripts/coccinelle/api/request_firmware-usermode.cocci index 94ab95cb7c75..1775fd3067e8 100644 --- a/scripts/coccinelle/api/request_firmware-usermode.cocci +++ b/scripts/coccinelle/api/request_firmware-usermode.cocci @@ -17,6 +17,13 @@ virtual report virtual context +@ r0 depends on report || context @ +declarer name DECLARE_FW_LOADER_USER; +expression E; +@@ + +DECLARE_FW_LOADER_USER(E); + @ r1 depends on report || context @ expression mod, name, dev, gfp, drv, cb; position p; @@ -30,7 +37,7 @@ position p; *request_firmware_nowait@p(mod, FW_ACTION_NOHOTPLUG, name, dev, gfp, drv, cb) ) -@script:python depends on report@ +@script:python depends on report && !r0 @ p << r1.p; @@ -- 2.9.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-07 0:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1466117661-22075-1-git-send-email-mcgrof@kernel.org>
2016-06-16 22:54 ` [PATCH v2 3/5] firmware: update usermode helper docs and add SmPL report Luis R. Rodriguez
2016-06-16 22:54 ` [PATCH v2 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation Luis R. Rodriguez
[not found] ` <1471999507-913-1-git-send-email-mcgrof@kernel.org>
2016-08-24 0:45 ` [PATCH v3 3/5] firmware: update usermode helper docs and add SmPL report mcgrof
2016-08-24 0:45 ` [PATCH v3 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation mcgrof
[not found] ` <1473208930-6835-1-git-send-email-mcgrof@kernel.org>
2016-09-07 0:42 ` [PATCH v4 3/5] firmware: update usermode helper docs and add SmPL report Luis R. Rodriguez
2016-09-07 0:42 ` [PATCH v4 4/5] firmware: add usermode helper DECLARE_FW_LOADER_USER() annotation Luis R. Rodriguez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).