| Message ID | 1431455457-25322-4-git-send-email-mcgrof@do-not-panic.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote: > > Instead of waiting until the last second to fail > on a request_firmware*() calls due to filename > truncation we can do an early check upon boot > and immediatley avoid any possible issues upfront. Why? This looks stupid. Why add this special case, when normal path lookup results in the proper errors? And if invalid pathnames are so frequent that this special case is somehow worth it, we should fix whoever generates that crap, instead of adding this insane special case. And if we don't handle the errors from normal path lookup properly, then we should fix *that*. In other words, I see absolutely no reason for this patch. Regardless of the reason for it, it seems entirely broken. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 12, 2015 at 01:35:59PM -0700, Linus Torvalds wrote: > On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez > <mcgrof@do-not-panic.com> wrote: > > > > Instead of waiting until the last second to fail > > on a request_firmware*() calls due to filename > > truncation we can do an early check upon boot > > and immediatley avoid any possible issues upfront. > > Why? This looks stupid. Why add this special case, when normal path > lookup results in the proper errors It seemed silly to proceed late if we can catch the possible name errors early. It does indeed have the cost of all that early cruft code. > And if invalid pathnames are so frequent that this special case is > somehow worth it, we should fix whoever generates that crap, instead > of adding this insane special case. OK, I'm all for ignoring non-upstream drivers. > And if we don't handle the errors from normal path lookup properly, > then we should fix *that*. That was done on patch 2, originally I was going for a simple early check which can be put on all API calls prior to doing anything too intrusive: +static int sysdata_validate_filename(const char *name) +{ + if (!name) + return -EINVAL; + /* POSIX.1 2.4: an empty pathname is invalid, match other checks */ + if (name[0] == '\0') + return -ENOENT; +} Since the truncation was possible too though it seemed worthy to add given that quite a few callers can end up re-using the same code. > In other words, I see absolutely no reason for this patch. Regardless > of the reason for it, it seems entirely broken. OK I'll get rid of all these early checks. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 99385fc..448388b 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -38,6 +38,20 @@ MODULE_AUTHOR("Manuel Estrada Sainz"); MODULE_DESCRIPTION("Multi purpose firmware loading support"); MODULE_LICENSE("GPL"); +static size_t __read_mostly max_sysdata_path_size; + +static int sysdata_validate_filename(const char *name) +{ + if (!name) + return -EINVAL; + /* POSIX.1 2.4: an empty pathname is invalid, match other checks */ + if (name[0] == '\0') + return -ENOENT; + if (strlen(name) > (PATH_MAX - max_sysdata_path_size)) + return -ENAMETOOLONG; + return 0; +} + /* Builtin firmware support */ #ifdef CONFIG_FW_LOADER @@ -1105,9 +1119,6 @@ _request_firmware(const struct firmware **firmware_p, const char *name, if (!firmware_p) return -EINVAL; - if (!name || name[0] == '\0') - return -EINVAL; - ret = _request_firmware_prepare(&fw, name, device); if (ret <= 0) /* error or already assigned */ goto out; @@ -1185,6 +1196,10 @@ request_firmware(const struct firmware **firmware_p, const char *name, { int ret; + ret = sysdata_validate_filename(name); + if (ret) + return ret; + /* Need to pin this module until return */ __module_get(THIS_MODULE); ret = _request_firmware(firmware_p, name, device, @@ -1210,6 +1225,10 @@ int request_firmware_direct(const struct firmware **firmware_p, { int ret; + ret = sysdata_validate_filename(name); + if (ret) + return ret; + __module_get(THIS_MODULE); ret = _request_firmware(firmware_p, name, device, FW_OPT_UEVENT | FW_OPT_NO_WARN); @@ -1288,8 +1307,13 @@ request_firmware_nowait( const char *name, struct device *device, gfp_t gfp, void *context, void (*cont)(const struct firmware *fw, void *context)) { + int ret; struct firmware_work *fw_work; + ret = sysdata_validate_filename(name); + if (ret) + return ret; + fw_work = kzalloc(sizeof(struct firmware_work), gfp); if (!fw_work) return -ENOMEM; @@ -1668,8 +1692,27 @@ static void __init fw_cache_init(void) #endif } +static size_t __init get_max_sysdata_path(void) +{ + size_t max_size = 0; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(fw_path); i++) { + size_t path_size; + /* skip the unset customized path */ + if (!fw_path[i][0]) + continue; + path_size = strlen(fw_path[i]); + if (path_size > max_size) + max_size = path_size; + } + + return max_size; +} + static int __init firmware_class_init(void) { + max_sysdata_path_size = get_max_sysdata_path(); fw_cache_init(); #ifdef CONFIG_FW_LOADER_USER_HELPER register_reboot_notifier(&fw_shutdown_nb);