[PATCH v1 0/4] firmware loader: cleanup and introduce search paths option

[PATCH v1 0/4] firmware loader: cleanup and introduce search paths option

[Ming Lei]

Hi,

The 1st patch cancels exporting of cache_firmware and uncache_firmware.

The 2nd one simplifies holding module for request_firmware().

The 3rd one introduces one kernel option to allow distributions or users
to set their specific firmware search paths.

The 4th one don't allow to request firmware via relative path.

V1:
	- 3/4: make code more clean, take the approach suggested by Takashi
	- 4/4: introduce 4/4	


 drivers/base/Kconfig          |   14 +++++++
 drivers/base/firmware_class.c |   91 ++++++++++++++++++++++++++++++++---------
 include/linux/firmware.h      |   11 -----
 3 files changed, 85 insertions(+), 31 deletions(-)

Thanks,
--
Ming Lei

[PATCH v1 1/4] firmware loader: don't export cache_firmware and uncache_firmware

[Ming Lei]

Looks no driver has the explict requirement for the two exported
API, just don't export them anymore.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |    6 ++----
 include/linux/firmware.h      |   11 -----------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 47e3a22..caaddef 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1244,7 +1244,7 @@ EXPORT_SYMBOL(request_firmware_nowait);
  * Return !0 otherwise
  *
  */
-int cache_firmware(const char *fw_name)
+static int cache_firmware(const char *fw_name)
 {
 	int ret;
 	const struct firmware *fw;
@@ -1259,7 +1259,6 @@ int cache_firmware(const char *fw_name)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(cache_firmware);
 
 /**
  * uncache_firmware - remove one cached firmware image
@@ -1272,7 +1271,7 @@ EXPORT_SYMBOL_GPL(cache_firmware);
  * Return !0 otherwise
  *
  */
-int uncache_firmware(const char *fw_name)
+static int uncache_firmware(const char *fw_name)
 {
 	struct firmware_buf *buf;
 	struct firmware fw;
@@ -1290,7 +1289,6 @@ int uncache_firmware(const char *fw_name)
 
 	return -EINVAL;
 }
-EXPORT_SYMBOL_GPL(uncache_firmware);
 
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index e4279fe..e154c10 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,8 +47,6 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
-int cache_firmware(const char *name);
-int uncache_firmware(const char *name);
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -68,15 +66,6 @@ static inline void release_firmware(const struct firmware *fw)
 {
 }
 
-static inline int cache_firmware(const char *name)
-{
-	return -ENOENT;
-}
-
-static inline int uncache_firmware(const char *name)
-{
-	return -EINVAL;
-}
 #endif
 
 #endif
-- 
1.7.9.5

[PATCH v1 2/4] firmware loader: simplify holding module for request_firmware

[Ming Lei]

module reference doesn't cover direct loading path, so this patch
simply holds the module in the whole life time of request_firmware()
to fix the problem.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index caaddef..6ede229 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -523,8 +523,6 @@ static void fw_dev_release(struct device *dev)
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 
 	kfree(fw_priv);
-
-	module_put(THIS_MODULE);
 }
 
 static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -852,9 +850,6 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	dev_set_uevent_suppress(f_dev, true);
 
-	/* Need to pin this module until class device is destroyed */
-	__module_get(THIS_MODULE);
-
 	retval = device_add(f_dev);
 	if (retval) {
 		dev_err(f_dev, "%s: device_register failed\n", __func__);
@@ -1131,7 +1126,13 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
                  struct device *device)
 {
-	return _request_firmware(firmware_p, name, device, true, false);
+	int ret;
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, true, false);
+	module_put(THIS_MODULE);
+	return ret;
 }
 EXPORT_SYMBOL(request_firmware);
 
-- 
1.7.9.5

[PATCH v1 3/4] firmware loader: allow distribution to choose default search paths

[Ming Lei]

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.

Also this patch introduces fw_get_fw_file_from_paths to cover all
search paths, and fw_get_filesystem_firmware is simpified a bit.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/Kconfig          |   14 ++++++++++
 drivers/base/firmware_class.c |   61 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 07abd9d..2be10e4 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -156,6 +156,20 @@ 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 colon like $PATH(e.g. on android,
+	  the option might look as "/etc/firmware:/vendor/firmware").
+	  Each path should be a absolute path, and relative path will be
+	  ignored.
+
+	  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 6ede229..051db83 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -265,8 +265,13 @@ static void fw_free_buf(struct firmware_buf *buf)
 
 /* direct firmware loading support */
 static char fw_path_para[256];
+
+/* search runtime paths first, then static pre-defined paths */
 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 +319,50 @@ 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;
+}
+
+/* The path in @paths is seperated by ';' */
+static bool fw_get_file_fw_from_paths(const char *paths, char *path,
+				      struct firmware_buf *buf)
+{
+	int len, start, end = -1;
+	char *pos;
+
+	do {
+		start = end + 1;
+		pos = strchr(&paths[start], ':');
+		if (pos) {
+			end = (int)(pos - paths);
+			len = end - start;
+		} else {
+			len = strlen(&paths[start]);
+		}
+
+		if (len <= 0 || PATH_MAX < len + 1 + 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;
+}
+
 static bool fw_get_filesystem_firmware(struct device *device,
 				       struct firmware_buf *buf)
 {
@@ -322,19 +371,11 @@ 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);
+		success = fw_get_file_fw_from_paths(fw_path[i], path,
+						    buf);
 		if (success)
 			break;
 	}
-- 
1.7.9.5

[RFC PATCH v1 4/4] firmware loader: don't allow to request firmware via relative path

[Ming Lei]

It isn't a good pratice to request firmware via relative path, also
might have security issue, so don't do it.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 051db83..d381db9 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -356,6 +356,10 @@ static bool fw_get_file_fw_from_paths(const char *paths, char *path,
 		strncpy(path, &paths[start], len);
 		snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
 
+		/* relative path isn't allowed */
+		if (strstr(path, "../"))
+			continue;
+
 		if (fw_get_file_firmware(path, buf))
 			return true;
 	} while (pos && end < strlen(paths) - 1);
@@ -1031,6 +1035,13 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 		return 0; /* assigned */
 	}
 
+	/* relative path isn't allowed */
+	if (strstr(name, "../")) {
+		dev_err(device, "%s: relative path isn't allowed\n",
+			name);
+		return -EINVAL;
+	}
+
 	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
 
 	/*
-- 
1.7.9.5

Re: [PATCH v1 3/4] firmware loader: allow distribution to choose default search paths

[Greg Kroah-Hartman]

On Thu, Jun 06, 2013 at 08:01:49PM +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.
> 
> Also this patch introduces fw_get_fw_file_from_paths to cover all
> search paths, and fw_get_filesystem_firmware is simpified a bit.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/Kconfig          |   14 ++++++++++
>  drivers/base/firmware_class.c |   61 ++++++++++++++++++++++++++++++++++-------
>  2 files changed, 65 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 07abd9d..2be10e4 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -156,6 +156,20 @@ 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 colon like $PATH(e.g. on android,
> +	  the option might look as "/etc/firmware:/vendor/firmware").
> +	  Each path should be a absolute path, and relative path will be
> +	  ignored.
> +
> +	  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 6ede229..051db83 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -265,8 +265,13 @@ static void fw_free_buf(struct firmware_buf *buf)
>  
>  /* direct firmware loading support */
>  static char fw_path_para[256];
> +
> +/* search runtime paths first, then static pre-defined paths */
>  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 +319,50 @@ 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;
> +}
> +
> +/* The path in @paths is seperated by ';' */

No it isn't.

> +static bool fw_get_file_fw_from_paths(const char *paths, char *path,
> +				      struct firmware_buf *buf)
> +{
> +	int len, start, end = -1;
> +	char *pos;
> +
> +	do {
> +		start = end + 1;
> +		pos = strchr(&paths[start], ':');

As you have an array of paths now, why are you doing the ':' check
still?  Don't do that, just allow the person building the kernel to add
one path to the loader, that should be all that we need, right?

thanks,

greg k-h

Re: [RFC PATCH v1 4/4] firmware loader: don't allow to request firmware via relative path

[Greg Kroah-Hartman]

On Thu, Jun 06, 2013 at 08:01:50PM +0800, Ming Lei wrote:
> It isn't a good pratice to request firmware via relative path, also
> might have security issue, so don't do it.

What would the security issue be?  You are letting the person who build
the kernel specify this, so they can put whatever they want in here, a
'..' isn't going to keep them from being able to do "bad" things if they
really want to.

thanks,

greg k-h

Re: [PATCH v1 3/4] firmware loader: allow distribution to choose default search paths

[Ming Lei]

On Fri, Jun 7, 2013 at 3:47 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 06, 2013 at 08:01:49PM +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.
>>
>> Also this patch introduces fw_get_fw_file_from_paths to cover all
>> search paths, and fw_get_filesystem_firmware is simpified a bit.
>>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  drivers/base/Kconfig          |   14 ++++++++++
>>  drivers/base/firmware_class.c |   61 ++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 65 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 07abd9d..2be10e4 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -156,6 +156,20 @@ 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 colon like $PATH(e.g. on android,
>> +       the option might look as "/etc/firmware:/vendor/firmware").
>> +       Each path should be a absolute path, and relative path will be
>> +       ignored.
>> +
>> +       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 6ede229..051db83 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -265,8 +265,13 @@ static void fw_free_buf(struct firmware_buf *buf)
>>
>>  /* direct firmware loading support */
>>  static char fw_path_para[256];
>> +
>> +/* search runtime paths first, then static pre-defined paths */
>>  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 +319,50 @@ 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;
>> +}
>> +
>> +/* The path in @paths is seperated by ';' */
>
> No it isn't.

Will fix it.

>
>> +static bool fw_get_file_fw_from_paths(const char *paths, char *path,
>> +                                   struct firmware_buf *buf)
>> +{
>> +     int len, start, end = -1;
>> +     char *pos;
>> +
>> +     do {
>> +             start = end + 1;
>> +             pos = strchr(&paths[start], ':');
>
> As you have an array of paths now, why are you doing the ':' check
> still?  Don't do that, just allow the person building the kernel to add
> one path to the loader, that should be all that we need, right?

The distribution may have put firmware under more than one places,
for example, android put firmwares under "/etc/firmware" and
"/vendor/firmware", so I am afraid that only allowing one path or two
paths isn't flexible enough.

Thanks,
--
Ming Lei

Re: [RFC PATCH v1 4/4] firmware loader: don't allow to request firmware via relative path

[Ming Lei]

On Fri, Jun 7, 2013 at 3:48 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 06, 2013 at 08:01:50PM +0800, Ming Lei wrote:
>> It isn't a good pratice to request firmware via relative path, also
>> might have security issue, so don't do it.
>
> What would the security issue be?  You are letting the person who build
> the kernel specify this, so they can put whatever they want in here, a
> '..' isn't going to keep them from being able to do "bad" things if they
> really want to.

In VM case, guest kernel might access host filesystem files via this trick,
but not sure if it is possible.

Takashi, could you explain the security issue of relative path?


Thanks,
--
Ming Lei

Re: [PATCH v1 3/4] firmware loader: allow distribution to choose default search paths

[Greg Kroah-Hartman]

On Fri, Jun 07, 2013 at 09:24:25AM +0800, Ming Lei wrote:
> On Fri, Jun 7, 2013 at 3:47 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jun 06, 2013 at 08:01:49PM +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.
> >>
> >> Also this patch introduces fw_get_fw_file_from_paths to cover all
> >> search paths, and fw_get_filesystem_firmware is simpified a bit.
> >>
> >> Cc: Takashi Iwai <tiwai@suse.de>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >> ---
> >>  drivers/base/Kconfig          |   14 ++++++++++
> >>  drivers/base/firmware_class.c |   61 ++++++++++++++++++++++++++++++++++-------
> >>  2 files changed, 65 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> >> index 07abd9d..2be10e4 100644
> >> --- a/drivers/base/Kconfig
> >> +++ b/drivers/base/Kconfig
> >> @@ -156,6 +156,20 @@ 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 colon like $PATH(e.g. on android,
> >> +       the option might look as "/etc/firmware:/vendor/firmware").
> >> +       Each path should be a absolute path, and relative path will be
> >> +       ignored.
> >> +
> >> +       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 6ede229..051db83 100644
> >> --- a/drivers/base/firmware_class.c
> >> +++ b/drivers/base/firmware_class.c
> >> @@ -265,8 +265,13 @@ static void fw_free_buf(struct firmware_buf *buf)
> >>
> >>  /* direct firmware loading support */
> >>  static char fw_path_para[256];
> >> +
> >> +/* search runtime paths first, then static pre-defined paths */
> >>  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 +319,50 @@ 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;
> >> +}
> >> +
> >> +/* The path in @paths is seperated by ';' */
> >
> > No it isn't.
> 
> Will fix it.
> 
> >
> >> +static bool fw_get_file_fw_from_paths(const char *paths, char *path,
> >> +                                   struct firmware_buf *buf)
> >> +{
> >> +     int len, start, end = -1;
> >> +     char *pos;
> >> +
> >> +     do {
> >> +             start = end + 1;
> >> +             pos = strchr(&paths[start], ':');
> >
> > As you have an array of paths now, why are you doing the ':' check
> > still?  Don't do that, just allow the person building the kernel to add
> > one path to the loader, that should be all that we need, right?
> 
> The distribution may have put firmware under more than one places,
> for example, android put firmwares under "/etc/firmware" and
> "/vendor/firmware", so I am afraid that only allowing one path or two
> paths isn't flexible enough.

How about we wait until someone complains about this?  Don't add
features that aren't needed, especially complex ones like this.

thanks,

greg k-h

Re: [RFC PATCH v1 4/4] firmware loader: don't allow to request firmware via relative path

[Greg Kroah-Hartman]

On Fri, Jun 07, 2013 at 09:30:09AM +0800, Ming Lei wrote:
> On Fri, Jun 7, 2013 at 3:48 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jun 06, 2013 at 08:01:50PM +0800, Ming Lei wrote:
> >> It isn't a good pratice to request firmware via relative path, also
> >> might have security issue, so don't do it.
> >
> > What would the security issue be?  You are letting the person who build
> > the kernel specify this, so they can put whatever they want in here, a
> > '..' isn't going to keep them from being able to do "bad" things if they
> > really want to.
> 
> In VM case, guest kernel might access host filesystem files via this trick,
> but not sure if it is possible.

How can a ".." do anything with guest kernels that total control over a
path not do?

thanks,

greg k-h

Re: [RFC PATCH v1 4/4] firmware loader: don't allow to request firmware via relative path

[Takashi Iwai]

At Fri, 7 Jun 2013 09:30:09 +0800,
Ming Lei wrote:
> 
> On Fri, Jun 7, 2013 at 3:48 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jun 06, 2013 at 08:01:50PM +0800, Ming Lei wrote:
> >> It isn't a good pratice to request firmware via relative path, also
> >> might have security issue, so don't do it.
> >
> > What would the security issue be?  You are letting the person who build
> > the kernel specify this, so they can put whatever they want in here, a
> > '..' isn't going to keep them from being able to do "bad" things if they
> > really want to.
> 
> In VM case, guest kernel might access host filesystem files via this trick,
> but not sure if it is possible.
> 
> Takashi, could you explain the security issue of relative path?

Well, I don't know, too.  My original question was what happens if you
pass a relative path to firmware_class.fw_path_para module option
(or the new kconfig)...


Takashi

Re: [RFC PATCH v1 4/4] firmware loader: don't allow to request firmware via relative path

[Ming Lei]

On Fri, Jun 7, 2013 at 2:04 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Fri, 7 Jun 2013 09:30:09 +0800,
> Ming Lei wrote:
>>
>> On Fri, Jun 7, 2013 at 3:48 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Jun 06, 2013 at 08:01:50PM +0800, Ming Lei wrote:
>> >> It isn't a good pratice to request firmware via relative path, also
>> >> might have security issue, so don't do it.
>> >
>> > What would the security issue be?  You are letting the person who build
>> > the kernel specify this, so they can put whatever they want in here, a
>> > '..' isn't going to keep them from being able to do "bad" things if they
>> > really want to.
>>
>> In VM case, guest kernel might access host filesystem files via this trick,
>> but not sure if it is possible.
>>
>> Takashi, could you explain the security issue of relative path?
>
> Well, I don't know, too.  My original question was what happens if you
> pass a relative path to firmware_class.fw_path_para module option
> (or the new kconfig)...

OK, so please ignore the patch.

Thanks
--
Ming Lei

Re: [PATCH v1 3/4] firmware loader: allow distribution to choose default search paths

[Ming Lei]

On Fri, Jun 7, 2013 at 12:20 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Jun 07, 2013 at 09:24:25AM +0800, Ming Lei wrote:
>> On Fri, Jun 7, 2013 at 3:47 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Jun 06, 2013 at 08:01:49PM +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.
>> >>
>> >> Also this patch introduces fw_get_fw_file_from_paths to cover all
>> >> search paths, and fw_get_filesystem_firmware is simpified a bit.
>> >>
>> >> Cc: Takashi Iwai <tiwai@suse.de>
>> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> >> ---
>> >>  drivers/base/Kconfig          |   14 ++++++++++
>> >>  drivers/base/firmware_class.c |   61 ++++++++++++++++++++++++++++++++++-------
>> >>  2 files changed, 65 insertions(+), 10 deletions(-)
>> >>
>> >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> >> index 07abd9d..2be10e4 100644
>> >> --- a/drivers/base/Kconfig
>> >> +++ b/drivers/base/Kconfig
>> >> @@ -156,6 +156,20 @@ 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 colon like $PATH(e.g. on android,
>> >> +       the option might look as "/etc/firmware:/vendor/firmware").
>> >> +       Each path should be a absolute path, and relative path will be
>> >> +       ignored.
>> >> +
>> >> +       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 6ede229..051db83 100644
>> >> --- a/drivers/base/firmware_class.c
>> >> +++ b/drivers/base/firmware_class.c
>> >> @@ -265,8 +265,13 @@ static void fw_free_buf(struct firmware_buf *buf)
>> >>
>> >>  /* direct firmware loading support */
>> >>  static char fw_path_para[256];
>> >> +
>> >> +/* search runtime paths first, then static pre-defined paths */
>> >>  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 +319,50 @@ 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;
>> >> +}
>> >> +
>> >> +/* The path in @paths is seperated by ';' */
>> >
>> > No it isn't.
>>
>> Will fix it.
>>
>> >
>> >> +static bool fw_get_file_fw_from_paths(const char *paths, char *path,
>> >> +                                   struct firmware_buf *buf)
>> >> +{
>> >> +     int len, start, end = -1;
>> >> +     char *pos;
>> >> +
>> >> +     do {
>> >> +             start = end + 1;
>> >> +             pos = strchr(&paths[start], ':');
>> >
>> > As you have an array of paths now, why are you doing the ':' check
>> > still?  Don't do that, just allow the person building the kernel to add
>> > one path to the loader, that should be all that we need, right?
>>
>> The distribution may have put firmware under more than one places,
>> for example, android put firmwares under "/etc/firmware" and
>> "/vendor/firmware", so I am afraid that only allowing one path or two
>> paths isn't flexible enough.
>
> How about we wait until someone complains about this?  Don't add
> features that aren't needed, especially complex ones like this.

That is fine with me.

But I believe soon or later people will ask for the option because
basically it will make direct loading to replace user space loader.
And now with the option, people can choose to disable
CONFIG_FW_LOADER_USER_HELPER to save code size if
their firmwares aren't under built-in paths.


Thanks,
--
Ming Lei

Re: [PATCH v1 2/4] firmware loader: simplify holding module for request_firmware

[Dmitry Torokhov]

Hi Ming,

On Thu, Jun 06, 2013 at 08:01:48PM +0800, Ming Lei wrote:
> module reference doesn't cover direct loading path, so this patch
> simply holds the module in the whole life time of request_firmware()
> to fix the problem.

This does not make sense to me. If request_firmware() is executing that
means some other module references it and module refcount already
reflects that.

We needed to pin module before Tejun's work ensuring that currently open
sysfs entries won't keep related kobjects pinned after kernel marked
them inactive. We can probably delete __module_get()/module_put() from
firmware_class.c now.

Thanks.

-- 
Dmitry

Re: [PATCH v1 2/4] firmware loader: simplify holding module for request_firmware

[Ming Lei]

Hi Dmitry,

On Tue, Jan 7, 2014 at 3:56 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Ming,
>
> On Thu, Jun 06, 2013 at 08:01:48PM +0800, Ming Lei wrote:
>> module reference doesn't cover direct loading path, so this patch
>> simply holds the module in the whole life time of request_firmware()
>> to fix the problem.
>
> This does not make sense to me. If request_firmware() is executing that
> means some other module references it and module refcount already
> reflects that.

Yes, you are right, holding this module references in both
request_firmware() and request_firmware_direct() shouldn't
be necessary.

>
> We needed to pin module before Tejun's work ensuring that currently open
> sysfs entries won't keep related kobjects pinned after kernel marked
> them inactive. We can probably delete __module_get()/module_put() from
> firmware_class.c now.

But holding the reference for request_firmware_nowait() is needed.


Thanks,
--
Ming Lei

Re: [PATCH v1 2/4] firmware loader: simplify holding module for request_firmware

[Dmitry Torokhov]

On Tue, Jan 07, 2014 at 10:53:57PM +0800, Ming Lei wrote:
> Hi Dmitry,
> 
> On Tue, Jan 7, 2014 at 3:56 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Ming,
> >
> > On Thu, Jun 06, 2013 at 08:01:48PM +0800, Ming Lei wrote:
> >> module reference doesn't cover direct loading path, so this patch
> >> simply holds the module in the whole life time of request_firmware()
> >> to fix the problem.
> >
> > This does not make sense to me. If request_firmware() is executing that
> > means some other module references it and module refcount already
> > reflects that.
> 
> Yes, you are right, holding this module references in both
> request_firmware() and request_firmware_direct() shouldn't
> be necessary.
> 
> >
> > We needed to pin module before Tejun's work ensuring that currently open
> > sysfs entries won't keep related kobjects pinned after kernel marked
> > them inactive. We can probably delete __module_get()/module_put() from
> > firmware_class.c now.
> 
> But holding the reference for request_firmware_nowait() is needed.

Ah, yes, of course.

Thanks.

-- 
Dmitry