Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd() - nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.) - module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com --- I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix? --- drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
- if (!name || name[0] == '\0') { + /* + * Reject firmware file names with "/../" sequences in them. + * There are drivers that construct firmware file names from + * device-supplied strings, and we don't want some device to be able + * to tell us "I would like to be sent my firmware from + * ../../../etc/shadow, please". + */ + if (!name || name[0] == '\0' || + strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) { ret = -EINVAL; goto out; }
--- base-commit: b0da640826ba3b6506b4996a6b23a429235e6923 change-id: 20240820-firmware-traversal-6df8501b0fe4
On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd()
- nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.)
- module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Your commit message very well describes the status quo, but only implies the problem, and skips how you intend to solve it.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com
I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix?
drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
- if (!name || name[0] == '\0') {
- /*
* Reject firmware file names with "/../" sequences in them.* There are drivers that construct firmware file names from* device-supplied strings, and we don't want some device to be able* to tell us "I would like to be sent my firmware from* ../../../etc/shadow, please".*/- if (!name || name[0] == '\0' ||
strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {
Seems reasonable, but are there any API users that rely on that?
I guess we can't just check for strstr(name, "../"), because "foo.." is a valid file name? Maybe it would be worth adding a comment and / or a small helper function for that.
I also suggest to update the documentation of the firmware loader API to let people know that going back the path isn't tolerated by this API.
ret = -EINVAL; goto out;}
base-commit: b0da640826ba3b6506b4996a6b23a429235e6923 change-id: 20240820-firmware-traversal-6df8501b0fe4 -- Jann Horn jannh@google.com
On Tue, Aug 20, 2024 at 02:14:11AM +0200, Danilo Krummrich wrote:
On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd()
- nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.)
- module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Your commit message very well describes the status quo, but only implies the problem, and skips how you intend to solve it.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com
I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix?
drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
- if (!name || name[0] == '\0') {
- /*
* Reject firmware file names with "/../" sequences in them.* There are drivers that construct firmware file names from* device-supplied strings, and we don't want some device to be able* to tell us "I would like to be sent my firmware from* ../../../etc/shadow, please".*/- if (!name || name[0] == '\0' ||
strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {Seems reasonable, but are there any API users that rely on that?
If so, then those all need to be fixed otherwise you will get terrible user experiences by just upgrading the kernel.
Luis
On Tue, Aug 20, 2024 at 2:14 AM Danilo Krummrich dakr@kernel.org wrote:
On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd()
- nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.)
- module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Your commit message very well describes the status quo, but only implies the problem, and skips how you intend to solve it.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com
I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix?
drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
if (!name || name[0] == '\0') {
/** Reject firmware file names with "/../" sequences in them.* There are drivers that construct firmware file names from* device-supplied strings, and we don't want some device to be able* to tell us "I would like to be sent my firmware from* ../../../etc/shadow, please".*/if (!name || name[0] == '\0' ||strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {Seems reasonable, but are there any API users that rely on that?
I tried grepping for in-kernel users and didn't find any, though I guess I could have missed something. I suppose slightly more likely than in-kernel users, there could be userspace code out there that intentionally uses netlink or sysfs interfaces to tell the kernel to load from firmware paths outside the firmware directory, though that would be kinda weird?
I guess we can't just check for strstr(name, "../"), because "foo.." is a valid file name?
Yeah, that's the intent.
Maybe it would be worth adding a comment and / or a small helper function for that.
Yeah, I guess that might make it clearer.
I also suggest to update the documentation of the firmware loader API to let people know that going back the path isn't tolerated by this API.
In Documentation/driver-api/firmware/request_firmware.rst, correct?
ret = -EINVAL; goto out; }
base-commit: b0da640826ba3b6506b4996a6b23a429235e6923 change-id: 20240820-firmware-traversal-6df8501b0fe4 -- Jann Horn jannh@google.com
On Tue, Aug 20, 2024 at 2:23 AM Jann Horn jannh@google.com wrote:
On Tue, Aug 20, 2024 at 2:14 AM Danilo Krummrich dakr@kernel.org wrote:
On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd()
- nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.)
- module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Your commit message very well describes the status quo, but only implies the problem, and skips how you intend to solve it.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com
I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix?
drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
if (!name || name[0] == '\0') {
/** Reject firmware file names with "/../" sequences in them.* There are drivers that construct firmware file names from* device-supplied strings, and we don't want some device to be able* to tell us "I would like to be sent my firmware from* ../../../etc/shadow, please".*/if (!name || name[0] == '\0' ||strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {Seems reasonable, but are there any API users that rely on that?
I tried grepping for in-kernel users and didn't find any, though I guess I could have missed something. I suppose slightly more likely than in-kernel users, there could be userspace code out there that intentionally uses netlink or sysfs interfaces to tell the kernel to load from firmware paths outside the firmware directory, though that would be kinda weird?
I guess if we are seriously concerned that someone might rely on that, there are several things we could do to mitigate it, ordered by increasing level of how annoying it would be to implement and how much it would nerf the check:
1. add a pr_warn() specifically for this case, so if it does break, users know what's wrong and can complain - I think I should probably do that in v2 anyway 2. add a module parameter to disable the check, so if it does break, users can immediately work around the issue 3. make the whole thing just a warning for now, and revisit it in a year or so to enable enforcement
My preference would be to implement number 1 but not 2/3, but if you think that's not enough to merge it, I could implement 2 or 3...
On Tue, Aug 20, 2024 at 02:42:46AM +0200, Jann Horn wrote:
On Tue, Aug 20, 2024 at 2:23 AM Jann Horn jannh@google.com wrote:
On Tue, Aug 20, 2024 at 2:14 AM Danilo Krummrich dakr@kernel.org wrote:
On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd()
- nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.)
- module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Your commit message very well describes the status quo, but only implies the problem, and skips how you intend to solve it.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com
I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix?
drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
if (!name || name[0] == '\0') {
/** Reject firmware file names with "/../" sequences in them.* There are drivers that construct firmware file names from* device-supplied strings, and we don't want some device to be able* to tell us "I would like to be sent my firmware from* ../../../etc/shadow, please".*/if (!name || name[0] == '\0' ||strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {Seems reasonable, but are there any API users that rely on that?
I tried grepping for in-kernel users and didn't find any, though I guess I could have missed something. I suppose slightly more likely than in-kernel users, there could be userspace code out there that intentionally uses netlink or sysfs interfaces to tell the kernel to load from firmware paths outside the firmware directory, though that would be kinda weird?
I guess if we are seriously concerned that someone might rely on that, there are several things we could do to mitigate it, ordered by increasing level of how annoying it would be to implement and how much it would nerf the check:
To me option 1 sounds sufficient and reasonable.
- add a pr_warn() specifically for this case, so if it does break,
users know what's wrong and can complain - I think I should probably do that in v2 anyway 2. add a module parameter to disable the check, so if it does break, users can immediately work around the issue 3. make the whole thing just a warning for now, and revisit it in a year or so to enable enforcement
My preference would be to implement number 1 but not 2/3, but if you think that's not enough to merge it, I could implement 2 or 3...
On Tue, Aug 20, 2024 at 02:42:46AM +0200, Jann Horn wrote:
On Tue, Aug 20, 2024 at 2:23 AM Jann Horn jannh@google.com wrote:
On Tue, Aug 20, 2024 at 2:14 AM Danilo Krummrich dakr@kernel.org wrote:
On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd()
- nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.)
- module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Your commit message very well describes the status quo, but only implies the problem, and skips how you intend to solve it.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com
I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix?
drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
if (!name || name[0] == '\0') {
/** Reject firmware file names with "/../" sequences in them.* There are drivers that construct firmware file names from* device-supplied strings, and we don't want some device to be able* to tell us "I would like to be sent my firmware from* ../../../etc/shadow, please".*/if (!name || name[0] == '\0' ||strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {Seems reasonable, but are there any API users that rely on that?
I tried grepping for in-kernel users and didn't find any, though I guess I could have missed something. I suppose slightly more likely than in-kernel users, there could be userspace code out there that intentionally uses netlink or sysfs interfaces to tell the kernel to load from firmware paths outside the firmware directory, though that would be kinda weird?
I guess if we are seriously concerned that someone might rely on that, there are several things we could do to mitigate it, ordered by increasing level of how annoying it would be to implement and how much it would nerf the check:
- add a pr_warn() specifically for this case, so if it does break,
users know what's wrong and can complain - I think I should probably do that in v2 anyway
That seems sane, at least provide a way to see what went wrong (or if someone is trying to do something "bad").
And yes, it should be backported to stable, so please leave that in there for your v2 patch.
Thanks for finding this!
greg k-h
On Tue, Aug 20, 2024 at 02:23:05AM +0200, Jann Horn wrote:
On Tue, Aug 20, 2024 at 2:14 AM Danilo Krummrich dakr@kernel.org wrote:
On Tue, Aug 20, 2024 at 01:18:54AM +0200, Jann Horn wrote:
Most firmware names are hardcoded strings, or are constructed from fairly constrained format strings where the dynamic parts are just some hex numbers or such.
However, there are a couple codepaths in the kernel where firmware file names contain string components that are passed through from a device or semi-privileged userspace; the ones I could find (not counting interfaces that require root privileges) are:
- lpfc_sli4_request_firmware_update() seems to construct the firmware filename from "ModelName", a string that was previously parsed out of some descriptor ("Vital Product Data") in lpfc_fill_vpd()
- nfp_net_fw_find() seems to construct a firmware filename from a model name coming from nfp_hwinfo_lookup(pf->hwinfo, "nffw.partno"), which I think parses some descriptor that was read from the device. (But this case likely isn't exploitable because the format string looks like "netronome/nic_%s", and there shouldn't be any *folders* starting with "netronome/nic_". The previous case was different because there, the "%s" is *at the start* of the format string.)
- module_flash_fw_schedule() is reachable from the ETHTOOL_MSG_MODULE_FW_FLASH_ACT netlink command, which is marked as GENL_UNS_ADMIN_PERM (meaning CAP_NET_ADMIN inside a user namespace is enough to pass the privilege check), and takes a userspace-provided firmware name. (But I think to reach this case, you need to have CAP_NET_ADMIN over a network namespace that a special kind of ethernet device is mapped into, so I think this is not a viable attack path in practice.)
For what it's worth, I went looking and haven't found any USB device drivers that use the firmware loader dangerously.
Your commit message very well describes the status quo, but only implies the problem, and skips how you intend to solve it.
Cc: stable@vger.kernel.org Fixes: abb139e75c2c ("firmware: teach the kernel to load firmware files directly from the filesystem") Signed-off-by: Jann Horn jannh@google.com
I wasn't sure whether to mark this one for stable or not - but I think since there seems to be at least one PCI device model which could trigger firmware loading with directory traversal, we should probably backport the fix?
drivers/base/firmware_loader/main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index a03ee4b11134..a32be64f3bf5 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -864,7 +864,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL;
if (!name || name[0] == '\0') {
/** Reject firmware file names with "/../" sequences in them.* There are drivers that construct firmware file names from* device-supplied strings, and we don't want some device to be able* to tell us "I would like to be sent my firmware from* ../../../etc/shadow, please".*/if (!name || name[0] == '\0' ||strstr(name, "/../") != NULL || strncmp(name, "../", 3) == 0) {Seems reasonable, but are there any API users that rely on that?
I tried grepping for in-kernel users and didn't find any, though I guess I could have missed something.
It's a bit hard to grep for, but I gave it a quick shot too and I can't find any results for "../" in combination with "fw", "path", "bin", etc. either.
I suppose slightly more likely than in-kernel users, there could be userspace code out there that intentionally uses netlink or sysfs interfaces to tell the kernel to load from firmware paths outside the firmware directory, though that would be kinda weird?
I agree it would be weird. Especially, since there is "firmware_class.path" available to avoid such hacks.
I guess we can't just check for strstr(name, "../"), because "foo.." is a valid file name?
Yeah, that's the intent.
Maybe it would be worth adding a comment and / or a small helper function for that.
Yeah, I guess that might make it clearer.
I also suggest to update the documentation of the firmware loader API to let people know that going back the path isn't tolerated by this API.
In Documentation/driver-api/firmware/request_firmware.rst, correct?
I think that's reasonable, though it would also be nice to have it in the documentation of the corresponding functions that take the argument.
Since all the request_firmware() derivates refer to request_firmware(), it's probably enough to add it there.
ret = -EINVAL; goto out; }
base-commit: b0da640826ba3b6506b4996a6b23a429235e6923 change-id: 20240820-firmware-traversal-6df8501b0fe4 -- Jann Horn jannh@google.com
linux-stable-mirror@lists.linaro.org
-
Danilo Krummrich -
Greg Kroah-Hartman -
Jann Horn -
Luis Chamberlain