Skip to contentSkip to conversation

firmware: make sure paths remain relative

PATCH Superseded Kees Cook 2 participants 6 messages Dec 14, 2012 – Dec 18, 2012 Driver core

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 firmware: make sure paths remain relative

Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to load firmware from an unexpected location. To minimize the risk of this, make sure the string "../" does not appear in the firmware name. This means that neither the users of request_firmware, nor the uevent handler have to do this filtering themselves.

---
 drivers/base/firmware_class.c |    3 +++
 1 file changed, 3 insertions(+)
drivers/base/firmware_class.c +3 -0
index d814603..9c4fb1c 100644
@@ -814,6 +814,9 @@ _request_firmware_prepare(const struct firmware **firmware_p, const char *name,
 	if (!firmware_p)
 		return ERR_PTR(-EINVAL);
 
+	if (strstr(name, "../"))
+		return ERR_PTR(-EINVAL);
+
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		dev_err(device, "%s: kmalloc(struct firmware) failed\n",
Signature · 6 lines
1.7.9.5


-- 
Kees Cook
Chrome OS Security
#1 lore raw
Quoted 2 lines · Kees Cook

Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to

I an wondering how the unprivileged userspace can write the firmware sysfs to trigger loading firmware?

Thanks,
--
Ming Lei

author
#2 lore raw
Quoted 6 lines · Ming Lei
Quoted 2 lines · Kees Cook

Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to

I an wondering how the unprivileged userspace can write the firmware sysfs to trigger loading firmware?

If a daemon were to, for example, make firmware selectable by the user (which under certain situations is possible in Chrome OS), it seems wasteful require these userspace tools/interfaces to each perform filtering, so I figured it would be trivial to put in here instead to avoid possible future vulnerabilities.

-Kees

Signature · 2 lines
Kees Cook
Chrome OS Security
#3 lore raw
Quoted 5 lines · Kees Cook
Quoted 4 lines · Ming Lei
Quoted 2 lines · Kees Cook

Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to

Quoted 8 lines
Quoted 2 lines

I an wondering how the unprivileged userspace can write the firmware sysfs to trigger loading firmware?

If a daemon were to, for example, make firmware selectable by the user (which under certain situations is possible in Chrome OS), it seems wasteful require these userspace tools/interfaces to each perform filtering, so I figured it would be trivial to put in here instead to avoid possible future vulnerabilities.

OK, I understand your concern, and looks reasonable wrt. the specific problem, and IMO, it is better to provide failure log so that the affected device driver can be fixed easily.

Thanks,
--
Ming Lei

author
#4 lore raw
Quoted 15 lines · Ming Lei
Quoted 5 lines · Kees Cook
Quoted 4 lines · Ming Lei
Quoted 2 lines · Kees Cook

Some devices have configurable firmware locations. If these configuration mechanisms are exposed to unprivileged userspace, it may be possible to

Quoted 8 lines
Quoted 2 lines

I an wondering how the unprivileged userspace can write the firmware sysfs to trigger loading firmware?

If a daemon were to, for example, make firmware selectable by the user (which under certain situations is possible in Chrome OS), it seems wasteful require these userspace tools/interfaces to each perform filtering, so I figured it would be trivial to put in here instead to avoid possible future vulnerabilities.

Quoted 3 lines

OK, I understand your concern, and looks reasonable wrt. the specific problem, and IMO, it is better to provide failure log so that the affected device driver can be fixed easily.

Do you mean a printk should be emitted on this error path? I can add that if so.

-Kees

Signature · 2 lines
Kees Cook
Chrome OS Security
Acked-by
#5 lore raw
Quoted 2 lines · Kees Cook

Do you mean a printk should be emitted on this error path? I can add that if so.

dev_err() should be better. With that, please feel free to add

Thanks,
--
Ming Lei