Messages in this thread | | | Date | Wed, 5 Jun 2013 17:59:35 +0800 | Subject | Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths | From | Ming Lei <> |
| |
On Wed, Jun 5, 2013 at 5:50 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 5 Jun 2013 17:35:26 +0800, > Ming Lei wrote: >> >> On Wed, Jun 5, 2013 at 3:10 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > At Wed, 5 Jun 2013 13:42:49 +0800, >> > Ming Lei wrote: >> >> >> >> For some distributions(e.g. android), firmware images aren't put >> >> under kernel built-in search paths, so introduce one Kconfig >> >> option to allow distributions or users to choose its specific default >> >> search paths, which are always tried before searching from kernel >> >> built-in paths in direct loading. >> >> >> >> Cc: Linus Torvalds <torvalds@linux-foundation.org> >> >> Cc: Takashi Iwai <tiwai@suse.de> >> >> Signed-off-by: Ming Lei <ming.lei@canonical.com> >> >> --- >> >> drivers/base/Kconfig | 12 +++++++ >> >> drivers/base/firmware_class.c | 76 ++++++++++++++++++++++++++++++++++++----- >> >> 2 files changed, 80 insertions(+), 8 deletions(-) >> >> >> >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig >> >> index 07abd9d..5b0c909 100644 >> >> --- a/drivers/base/Kconfig >> >> +++ b/drivers/base/Kconfig >> >> @@ -156,6 +156,18 @@ config FW_LOADER_USER_HELPER >> >> no longer required unless you have a special firmware file that >> >> resides in a non-standard path. >> >> >> >> +config FW_CUSTOMIZED_PATH >> >> + string "default firmware search paths for direct loading" >> >> + help >> >> + On some distribution(e.g. android), firmware images aren't >> >> + put under kernel built-in search paths, so provide this option >> >> + for distributions to choose a distribution specific firmware >> >> + search path. The option allows to choose more than one path, >> >> + and paths are seperated with semicolon(e.g. on android, the >> >> + option might look as "/etc/firmware;/vendor/firmware"). >> >> + >> >> + If you are unsure about this, don't choose here. >> >> + >> >> config DEBUG_DRIVER >> >> bool "Driver Core verbose debug messages" >> >> depends on DEBUG_KERNEL >> >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c >> >> index c743409..50b5913 100644 >> >> --- a/drivers/base/firmware_class.c >> >> +++ b/drivers/base/firmware_class.c >> >> @@ -267,6 +267,9 @@ static void fw_free_buf(struct firmware_buf *buf) >> >> static char fw_path_para[256]; >> >> static const char * const fw_path[] = { >> >> fw_path_para, >> >> +#ifdef CONFIG_FW_CUSTOMIZED_PATH >> >> + CONFIG_FW_CUSTOMIZED_PATH, >> >> +#endif >> >> "/lib/firmware/updates/" UTS_RELEASE, >> >> "/lib/firmware/updates", >> >> "/lib/firmware/" UTS_RELEASE, >> >> @@ -314,6 +317,59 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf >> >> return true; >> >> } >> >> >> >> +static bool fw_get_file_firmware(const char *path, >> >> + struct firmware_buf *buf) >> >> +{ >> >> + struct file *file; >> >> + bool success; >> >> + >> >> + file = filp_open(path, O_RDONLY, 0); >> >> + if (IS_ERR(file)) >> >> + return false; >> >> + success = fw_read_file_contents(file, buf); >> >> + fput(file); >> >> + >> >> + return success; >> >> +} >> >> + >> >> +#ifdef CONFIG_FW_CUSTOMIZED_PATH >> >> +/* The path in @paths is seperated by ';' */ >> >> +static bool fw_get_fw_file_from_paths(const char *paths, char *path, >> >> + struct firmware_buf *buf) >> >> +{ >> >> + int len, start, end; >> >> + char *pos; >> >> + >> >> + end = -1; >> >> + do { >> >> + start = end + 1; >> >> + pos = strchr(&paths[start], ';'); >> >> + if (pos) { >> >> + end = (int)(pos - paths); >> >> + len = end - start; >> >> + } else { >> >> + len = strlen(&paths[start]); >> >> + } >> >> + >> >> + if (PATH_MAX < len + strlen(buf->fw_id)) >> >> + continue; >> >> + strncpy(path, &paths[start], len); >> >> + snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id); >> >> + >> >> + if (fw_get_file_firmware(path, buf)) >> >> + return true; >> >> + } while (pos && end < strlen(paths) - 1); >> >> + >> >> + return false; >> >> +} >> >> +#else >> >> +static bool fw_get_fw_file_from_paths(const char *paths, char *path, >> >> + struct firmware_buf *buf) >> >> +{ >> >> + return false; >> >> +} >> >> +#endif >> >> + >> >> static bool fw_get_filesystem_firmware(struct device *device, >> >> struct firmware_buf *buf) >> >> { >> >> @@ -322,19 +378,23 @@ static bool fw_get_filesystem_firmware(struct device *device, >> >> char *path = __getname(); >> >> >> >> for (i = 0; i < ARRAY_SIZE(fw_path); i++) { >> >> - struct file *file; >> >> >> >> /* skip the unset customized path */ >> >> if (!fw_path[i][0]) >> >> continue; >> >> >> >> - snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id); >> >> - >> >> - file = filp_open(path, O_RDONLY, 0); >> >> - if (IS_ERR(file)) >> >> - continue; >> >> - success = fw_read_file_contents(file, buf); >> >> - fput(file); >> >> + /* >> >> + * If CONFIG_FW_CUSTOMIZED_PATH is set, search from >> >> + * these paths first >> >> + */ >> >> + if (i == 1 && ARRAY_SIZE(fw_path) > 5) { >> >> + success = fw_get_fw_file_from_paths(fw_path[1], >> >> + path, buf); >> >> + } else { >> >> + snprintf(path, PATH_MAX, "%s/%s", fw_path[i], >> >> + buf->fw_id); >> >> + success = fw_get_file_firmware(path, buf); >> > >> > Shouldn't fw_get_fw_file_from_paths() be applied unconditionally? >> > It'll be benefit for the customized path passed via module option, >> > too. The only drawback is a slight code growth, but the code can be >> > reduced a bit with strcspn() or such. >> >> You mean that both the 1st two items should be covered by >> fw_get_fw_file_from_paths()? If so, the function may become >> a bit ugly since two strings are required to pass in, and we can't >> merge one runtime string and one ro string created in compiling. > > I meant to simply call fw_get_fw_file_from_paths() for all fw_path[] > entries. So far, the module option can pass only a single path. > But if it's handled through fw_get_file_from_paths(), you can pass > multiple paths there, too. > >> Looks we can let fw_get_fw_file_from_paths() handle all >> predefined paths(CONFIG_FW_CUSTOMIZED_PATH and >> kernel built-in paths), then fw_get_filesystem_firmware() >> may become simple, just check fw_path_para and all >> other paths by fw_get_fw_file_from_paths(). How about the >> idea? >> >> > BTW, I now wonder what happens if you pass a relative path. >> > Did you already test it? >> >> I tested absolute paths, and not test relative paths. Do you mean >> it may cause security problem? > > Yes. It just came to my mind while reviewing your patch. > >> If so, we can check and ignore them, >> but it should be OK since the paths are provided by kernel builder. >> >From view of function, I don't think there are much difference with >> absolute paths. > > The path can be provided via module option, too, so we need to check > it in anyway.
Yes, even the firmware name may includes "..".
Thanks, -- Ming Lei
| |