[PATCH 0/3] Fix FORTIFY=y UBSAN_LOCAL_BOUNDS=y

[PATCH 0/3] Fix FORTIFY=y UBSAN_LOCAL_BOUNDS=y

[Nick Desaulniers]

With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we
observe a runtime panic while running Android's Compatibility Test
Suite's (CTS) android.hardware.input.cts.tests.  This is stemming from a
strlen() call in hidinput_allocate().

__builtin_object_size(str, 0 or 1) has interesting behavior for C
strings when str is runtime dependent, and all possible values are known
at compile time; it evaluates to the maximum of those sizes. This causes
UBSAN_LOCAL_BOUNDS to insert faults for the smaller values, which we
trip at runtime.

Patch 1 adds a KCONFIG version check for __builtin_dynamic_object_size,
and uses that in __compiletime_strlen rather than __builtin_object_size.
Patch 2 and 3 are cosmetic cleanups, they're not as important to me as
patch 1 is.

Nick Desaulniers (3):
  fortify: use __builtin_dynamic_object_size in __compiletime_strlen
  fortify: cosmetic cleanups to __compiletime_strlen
  HID: avoid runtime call to strlen

 drivers/hid/hid-input.c        | 13 ++++++++++++-
 include/linux/fortify-string.h | 15 ++++++++++-----
 init/Kconfig                   |  3 +++
 3 files changed, 25 insertions(+), 6 deletions(-)

-- 
2.37.2.672.g94769d06f0-goog

[PATCH 1/3] fortify: use __builtin_dynamic_object_size in __compiletime_strlen

[Nick Desaulniers]

With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we
observe a runtime panic while running Android's Compatibility Test
Suite's (CTS) android.hardware.input.cts.tests.  This is stemming from a
strlen() call in hidinput_allocate().

__compiletime_strlen is implemented in terms of __builtin_object_size(),
then does an array access to check for NUL-termination. A quirk of
__builtin_object_size() is that for strings whose values are runtime
dependent, __builtin_object_size(str, 1 or 0) returns the maximum size
of possible values when those sizes are determinable at compile time.
Example:

  static const char *v = "FOO BAR";
  static const char *y = "FOO BA";
  unsigned long x (int z) {
      // Returns 8, which is:
      // max(__builtin_object_size(v, 1), __builtin_object_size(y, 1))
      return __builtin_object_size(z ? v : y, 1);
  }

So when FORTIFY is enabled, the current implementation of
__compiletime_strlen will try to access beyond the end of y at runtime
using the size of v. Mixed with UBSAN_LOCAL_BOUNDS we get a fault.

hidinput_allocate() has a local C string whose value is control flow
dependent on a switch statement, so __builtin_object_size(str, 1)
evaluates to the maximum string length, making all other cases fault on
the last character check. hidinput_allocate() could be cleaned up to
avoid runtime calls to strlen() since the local variable can only have
literal values, so there's no benefit to trying to fortify the strlen
call site there.

Add a Kconfig check for __builtin_dynamic_object_size(), then use that
when available (gcc-12+, all supported versions of clang) which avoids
this surprising behavior.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 include/linux/fortify-string.h | 8 +++++++-
 init/Kconfig                   | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 3b401fa0f374..c5adad596a3f 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -14,11 +14,17 @@ void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("
 void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
 void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");
 
+#ifdef CONFIG_CC_HAS_BUILTIN_DYNAMIC_OBJECT_SIZE
+#define __object_size __builtin_dynamic_object_size
+#else
+#define __object_size __builtin_object_size
+#endif
+
 #define __compiletime_strlen(p)					\
 ({								\
 	unsigned char *__p = (unsigned char *)(p);		\
 	size_t __ret = (size_t)-1;				\
-	size_t __p_size = __builtin_object_size(p, 1);		\
+	size_t __p_size = __object_size(p, 1);			\
 	if (__p_size != (size_t)-1) {				\
 		size_t __p_len = __p_size - 1;			\
 		if (__builtin_constant_p(__p[__p_len]) &&	\
diff --git a/init/Kconfig b/init/Kconfig
index 532362fcfe31..87dd31aa54ad 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -876,6 +876,9 @@ config ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
 config CC_HAS_INT128
 	def_bool !$(cc-option,$(m64-flag) -D__SIZEOF_INT128__=0) && 64BIT
 
+config CC_HAS_BUILTIN_DYNAMIC_OBJECT_SIZE
+	def_bool !CC_IS_GCC || GCC_VERSION >= 120000
+
 config CC_IMPLICIT_FALLTHROUGH
 	string
 	default "-Wimplicit-fallthrough=5" if CC_IS_GCC && $(cc-option,-Wimplicit-fallthrough=5)
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 2/3] fortify: cosmetic cleanups to __compiletime_strlen

[Nick Desaulniers]

Two things I noticed in __compiletime_strlen:
1. A temporary, __p, is created+used to avoid repeated side effects from
   multiple evaluation of the macro parameter, but the macro parameter
   was being used accidentally in __builtin_object_size.
2. The temporary has a curious signedness and const-less qualification.
   Just use __auto_type.
3. (size_t)-1 is perhaps more readable as -1UL.
4. __p_size == -1UL when __builtin_object_size can't evaluate the
   object size at compile time. We could just reuse __ret and use one
   less variable here.

Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 include/linux/fortify-string.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index c5adad596a3f..aaf73575050f 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -22,11 +22,10 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("
 
 #define __compiletime_strlen(p)					\
 ({								\
-	unsigned char *__p = (unsigned char *)(p);		\
-	size_t __ret = (size_t)-1;				\
-	size_t __p_size = __object_size(p, 1);			\
-	if (__p_size != (size_t)-1) {				\
-		size_t __p_len = __p_size - 1;			\
+	__auto_type __p = (p);					\
+	size_t __ret = __object_size(__p, 1);			\
+	if (__ret != -1UL) {					\
+		size_t __p_len = __ret - 1;			\
 		if (__builtin_constant_p(__p[__p_len]) &&	\
 		    __p[__p_len] == '\0')			\
 			__ret = __builtin_strlen(__p);		\
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 3/3] HID: avoid runtime call to strlen

[Nick Desaulniers]

While looking into a CONFIG_FORTIFY=y related bug, I noticed that
hid_allocate calls strlen() on a local C string variable. This variable
can only have literal string values. There is no benefit to having
FORTIFY have this be a checked strlen call, because these are literal
values.  By calling strlen() explicitly in the branches of a switch, the
compiler can evaluate strlen("literal value") at compile time, rather
than at runtime.

Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 drivers/hid/hid-input.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 48c1c02c69f4..9ad3cc88c26b 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1922,12 +1922,15 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid,
 		switch (application) {
 		case HID_GD_KEYBOARD:
 			suffix = "Keyboard";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_GD_KEYPAD:
 			suffix = "Keypad";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_GD_MOUSE:
 			suffix = "Mouse";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_DG_PEN:
 			/*
@@ -1938,36 +1941,44 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid,
 			 * will have to change it and the test suite will not be happy.
 			 */
 			suffix = "Stylus";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_DG_STYLUS:
 			suffix = "Pen";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_DG_TOUCHSCREEN:
 			suffix = "Touchscreen";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_DG_TOUCHPAD:
 			suffix = "Touchpad";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_GD_SYSTEM_CONTROL:
 			suffix = "System Control";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_CP_CONSUMER_CONTROL:
 			suffix = "Consumer Control";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_GD_WIRELESS_RADIO_CTLS:
 			suffix = "Wireless Radio Control";
+			suffix_len = strlen(suffix);
 			break;
 		case HID_GD_SYSTEM_MULTIAXIS:
 			suffix = "System Multi Axis";
+			suffix_len = strlen(suffix);
 			break;
 		default:
+			suffix_len = 0;
 			break;
 		}
 	}
 
 	if (suffix) {
 		name_len = strlen(hid->name);
-		suffix_len = strlen(suffix);
 		if ((name_len < suffix_len) ||
 		    strcmp(hid->name + name_len - suffix_len, suffix)) {
 			hidinput->name = kasprintf(GFP_KERNEL, "%s %s",
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH 3/3] HID: avoid runtime call to strlen

[Greg KH]

On Tue, Aug 30, 2022 at 01:53:09PM -0700, Nick Desaulniers wrote:
> While looking into a CONFIG_FORTIFY=y related bug, I noticed that
> hid_allocate calls strlen() on a local C string variable. This variable
> can only have literal string values. There is no benefit to having
> FORTIFY have this be a checked strlen call, because these are literal
> values.  By calling strlen() explicitly in the branches of a switch, the
> compiler can evaluate strlen("literal value") at compile time, rather
> than at runtime.
> 
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  drivers/hid/hid-input.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 48c1c02c69f4..9ad3cc88c26b 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1922,12 +1922,15 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid,
>  		switch (application) {
>  		case HID_GD_KEYBOARD:
>  			suffix = "Keyboard";
> +			suffix_len = strlen(suffix);
>  			break;
>  		case HID_GD_KEYPAD:
>  			suffix = "Keypad";
> +			suffix_len = strlen(suffix);
>  			break;
>  		case HID_GD_MOUSE:
>  			suffix = "Mouse";
> +			suffix_len = strlen(suffix);

<snip>

This seems ripe for someone to come along and go "look at this cleanup
patch where I move all of this duplicated code to below it in one line!"

As this is a compiler bug, why not fix the compiler?  Or at least put a
comment in here saying why this looks so odd to prevent it from being
changed in the future.

thanks,

greg k-h

Re: [PATCH 2/3] fortify: cosmetic cleanups to __compiletime_strlen

[kernel test robot]

Hi Nick,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kees/for-next/hardening]
[also build test WARNING on kees/for-next/pstore hid/for-next linus/master v6.0-rc3 next-20220831]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nick-Desaulniers/Fix-FORTIFY-y-UBSAN_LOCAL_BOUNDS-y/20220831-045536
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening
config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220831/202208312049.JxYsAiD2-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/ab66e7d1d8a90f6addac0da9b3ae13d77f095f76
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nick-Desaulniers/Fix-FORTIFY-y-UBSAN_LOCAL_BOUNDS-y/20220831-045536
        git checkout ab66e7d1d8a90f6addac0da9b3ae13d77f095f76
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/mtd/ubi/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/mtd/ubi/cdev.c: In function 'rename_volumes':
>> drivers/mtd/ubi/cdev.c:710:61: warning: array subscript -1 is below array bounds of 'struct <anonymous>[32]' [-Warray-bounds]
     710 |                         if (req->ents[i].vol_id == req->ents[n].vol_id) {
         |                                                    ~~~~~~~~~^~~
   In file included from drivers/mtd/ubi/cdev.c:33:
   include/uapi/mtd/ubi-user.h:404:11: note: while referencing 'ents'
     404 |         } ents[UBI_MAX_RNVOL];
         |           ^~~~
   drivers/mtd/ubi/cdev.c:715:68: warning: array subscript -1 is below array bounds of 'struct <anonymous>[32]' [-Warray-bounds]
     715 |                         if (!strcmp(req->ents[i].name, req->ents[n].name)) {
         |                                                        ~~~~~~~~~~~~^~~~~
   In file included from drivers/mtd/ubi/cdev.c:33:
   include/uapi/mtd/ubi-user.h:404:11: note: while referencing 'ents'
     404 |         } ents[UBI_MAX_RNVOL];
         |           ^~~~


vim +710 drivers/mtd/ubi/cdev.c

801c135ce73d5d Artem B. Bityutskiy 2006-06-27  668  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  669  /**
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  670   * rename_volumes - rename UBI volumes.
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  671   * @ubi: UBI device description object
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  672   * @req: volumes re-name request
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  673   *
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  674   * This is a helper function for the volume re-name IOCTL which validates the
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  675   * the request, opens the volume and calls corresponding volumes management
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  676   * function. Returns zero in case of success and a negative error code in case
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  677   * of failure.
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  678   */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  679  static int rename_volumes(struct ubi_device *ubi,
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  680  			  struct ubi_rnvol_req *req)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  681  {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  682  	int i, n, err;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  683  	struct list_head rename_list;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  684  	struct ubi_rename_entry *re, *re1;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  685  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  686  	if (req->count < 0 || req->count > UBI_MAX_RNVOL)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  687  		return -EINVAL;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  688  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  689  	if (req->count == 0)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  690  		return 0;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  691  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  692  	/* Validate volume IDs and names in the request */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  693  	for (i = 0; i < req->count; i++) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  694  		if (req->ents[i].vol_id < 0 ||
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  695  		    req->ents[i].vol_id >= ubi->vtbl_slots)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  696  			return -EINVAL;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  697  		if (req->ents[i].name_len < 0)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  698  			return -EINVAL;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  699  		if (req->ents[i].name_len > UBI_VOL_NAME_MAX)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  700  			return -ENAMETOOLONG;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  701  		req->ents[i].name[req->ents[i].name_len] = '\0';
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  702  		n = strlen(req->ents[i].name);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  703  		if (n != req->ents[i].name_len)
7fbbd05799976c Dan Carpenter       2014-09-19  704  			return -EINVAL;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  705  	}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  706  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  707  	/* Make sure volume IDs and names are unique */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  708  	for (i = 0; i < req->count - 1; i++) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  709  		for (n = i + 1; n < req->count; n++) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13 @710  			if (req->ents[i].vol_id == req->ents[n].vol_id) {
326087033108e7 Tanya Brokhman      2014-10-20  711  				ubi_err(ubi, "duplicated volume id %d",
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  712  					req->ents[i].vol_id);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  713  				return -EINVAL;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  714  			}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  715  			if (!strcmp(req->ents[i].name, req->ents[n].name)) {
326087033108e7 Tanya Brokhman      2014-10-20  716  				ubi_err(ubi, "duplicated volume name \"%s\"",
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  717  					req->ents[i].name);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  718  				return -EINVAL;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  719  			}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  720  		}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  721  	}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  722  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  723  	/* Create the re-name list */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  724  	INIT_LIST_HEAD(&rename_list);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  725  	for (i = 0; i < req->count; i++) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  726  		int vol_id = req->ents[i].vol_id;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  727  		int name_len = req->ents[i].name_len;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  728  		const char *name = req->ents[i].name;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  729  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  730  		re = kzalloc(sizeof(struct ubi_rename_entry), GFP_KERNEL);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  731  		if (!re) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  732  			err = -ENOMEM;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  733  			goto out_free;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  734  		}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  735  
892abde56c1c5a Richard Weinberger  2014-11-24  736  		re->desc = ubi_open_volume(ubi->ubi_num, vol_id, UBI_METAONLY);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  737  		if (IS_ERR(re->desc)) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  738  			err = PTR_ERR(re->desc);
326087033108e7 Tanya Brokhman      2014-10-20  739  			ubi_err(ubi, "cannot open volume %d, error %d",
326087033108e7 Tanya Brokhman      2014-10-20  740  				vol_id, err);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  741  			kfree(re);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  742  			goto out_free;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  743  		}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  744  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  745  		/* Skip this re-naming if the name does not really change */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  746  		if (re->desc->vol->name_len == name_len &&
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  747  		    !memcmp(re->desc->vol->name, name, name_len)) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  748  			ubi_close_volume(re->desc);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  749  			kfree(re);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  750  			continue;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  751  		}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  752  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  753  		re->new_name_len = name_len;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  754  		memcpy(re->new_name, name, name_len);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  755  		list_add_tail(&re->list, &rename_list);
719bb84017fcfc Artem Bityutskiy    2012-08-27  756  		dbg_gen("will rename volume %d from \"%s\" to \"%s\"",
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  757  			vol_id, re->desc->vol->name, name);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  758  	}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  759  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  760  	if (list_empty(&rename_list))
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  761  		return 0;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  762  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  763  	/* Find out the volumes which have to be removed */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  764  	list_for_each_entry(re, &rename_list, list) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  765  		struct ubi_volume_desc *desc;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  766  		int no_remove_needed = 0;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  767  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  768  		/*
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  769  		 * Volume @re->vol_id is going to be re-named to
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  770  		 * @re->new_name, while its current name is @name. If a volume
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  771  		 * with name @re->new_name currently exists, it has to be
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  772  		 * removed, unless it is also re-named in the request (@req).
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  773  		 */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  774  		list_for_each_entry(re1, &rename_list, list) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  775  			if (re->new_name_len == re1->desc->vol->name_len &&
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  776  			    !memcmp(re->new_name, re1->desc->vol->name,
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  777  				    re1->desc->vol->name_len)) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  778  				no_remove_needed = 1;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  779  				break;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  780  			}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  781  		}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  782  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  783  		if (no_remove_needed)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  784  			continue;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  785  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  786  		/*
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  787  		 * It seems we need to remove volume with name @re->new_name,
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  788  		 * if it exists.
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  789  		 */
f2863c54f30ccc Artem Bityutskiy    2008-12-28  790  		desc = ubi_open_volume_nm(ubi->ubi_num, re->new_name,
f2863c54f30ccc Artem Bityutskiy    2008-12-28  791  					  UBI_EXCLUSIVE);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  792  		if (IS_ERR(desc)) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  793  			err = PTR_ERR(desc);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  794  			if (err == -ENODEV)
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  795  				/* Re-naming into a non-existing volume name */
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  796  				continue;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  797  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  798  			/* The volume exists but busy, or an error occurred */
326087033108e7 Tanya Brokhman      2014-10-20  799  			ubi_err(ubi, "cannot open volume \"%s\", error %d",
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  800  				re->new_name, err);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  801  			goto out_free;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  802  		}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  803  
01ebc12f5f2e88 Julia Lawall        2010-08-07  804  		re1 = kzalloc(sizeof(struct ubi_rename_entry), GFP_KERNEL);
01ebc12f5f2e88 Julia Lawall        2010-08-07  805  		if (!re1) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  806  			err = -ENOMEM;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  807  			ubi_close_volume(desc);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  808  			goto out_free;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  809  		}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  810  
01ebc12f5f2e88 Julia Lawall        2010-08-07  811  		re1->remove = 1;
01ebc12f5f2e88 Julia Lawall        2010-08-07  812  		re1->desc = desc;
01ebc12f5f2e88 Julia Lawall        2010-08-07  813  		list_add(&re1->list, &rename_list);
719bb84017fcfc Artem Bityutskiy    2012-08-27  814  		dbg_gen("will remove volume %d, name \"%s\"",
01ebc12f5f2e88 Julia Lawall        2010-08-07  815  			re1->desc->vol->vol_id, re1->desc->vol->name);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  816  	}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  817  
f089c0b28cdba1 Artem Bityutskiy    2009-05-07  818  	mutex_lock(&ubi->device_mutex);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  819  	err = ubi_rename_volumes(ubi, &rename_list);
f089c0b28cdba1 Artem Bityutskiy    2009-05-07  820  	mutex_unlock(&ubi->device_mutex);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  821  
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  822  out_free:
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  823  	list_for_each_entry_safe(re, re1, &rename_list, list) {
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  824  		ubi_close_volume(re->desc);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  825  		list_del(&re->list);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  826  		kfree(re);
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  827  	}
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  828  	return err;
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  829  }
f40ac9cdf69912 Artem Bityutskiy    2008-07-13  830  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 1/3] fortify: use __builtin_dynamic_object_size in __compiletime_strlen

[Kees Cook]

On Tue, Aug 30, 2022 at 01:53:07PM -0700, Nick Desaulniers wrote:
> With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we
> observe a runtime panic while running Android's Compatibility Test
> Suite's (CTS) android.hardware.input.cts.tests.  This is stemming from a
> strlen() call in hidinput_allocate().
> 
> __compiletime_strlen is implemented in terms of __builtin_object_size(),
> then does an array access to check for NUL-termination. A quirk of
> __builtin_object_size() is that for strings whose values are runtime
> dependent, __builtin_object_size(str, 1 or 0) returns the maximum size
> of possible values when those sizes are determinable at compile time.
> Example:
> 
>   static const char *v = "FOO BAR";
>   static const char *y = "FOO BA";
>   unsigned long x (int z) {
>       // Returns 8, which is:
>       // max(__builtin_object_size(v, 1), __builtin_object_size(y, 1))
>       return __builtin_object_size(z ? v : y, 1);
>   }
> 
> So when FORTIFY is enabled, the current implementation of
> __compiletime_strlen will try to access beyond the end of y at runtime
> using the size of v. Mixed with UBSAN_LOCAL_BOUNDS we get a fault.
> 
> hidinput_allocate() has a local C string whose value is control flow
> dependent on a switch statement, so __builtin_object_size(str, 1)
> evaluates to the maximum string length, making all other cases fault on
> the last character check. hidinput_allocate() could be cleaned up to
> avoid runtime calls to strlen() since the local variable can only have
> literal values, so there's no benefit to trying to fortify the strlen
> call site there.
> 
> Add a Kconfig check for __builtin_dynamic_object_size(), then use that
> when available (gcc-12+, all supported versions of clang) which avoids
> this surprising behavior.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  include/linux/fortify-string.h | 8 +++++++-
>  init/Kconfig                   | 3 +++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 3b401fa0f374..c5adad596a3f 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -14,11 +14,17 @@ void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("
>  void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
>  void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");
>  
> +#ifdef CONFIG_CC_HAS_BUILTIN_DYNAMIC_OBJECT_SIZE
> +#define __object_size __builtin_dynamic_object_size
> +#else
> +#define __object_size __builtin_object_size
> +#endif

Instead of a Kconfig, how about just:

#if __has_builtin(__builtin_dynamic_object_size)
# define __object_size __builtin_dynamic_object_size
#else
# define __object_size __builtin_object_size
#endif

?

> +
>  #define __compiletime_strlen(p)					\
>  ({								\
>  	unsigned char *__p = (unsigned char *)(p);		\
>  	size_t __ret = (size_t)-1;				\
> -	size_t __p_size = __builtin_object_size(p, 1);		\
> +	size_t __p_size = __object_size(p, 1);			\
>  	if (__p_size != (size_t)-1) {				\
>  		size_t __p_len = __p_size - 1;			\
>  		if (__builtin_constant_p(__p[__p_len]) &&	\

The fact that __builtin_object_size() will actually span control flow,
and produce a size-inclusive result on the possible inputs is ...
surprising and potentially quite problematic. But I'm satisfied that
bdos appears to fix it here (since the "compiletime"ness will still get
filtered by the __builtin_constant_p() check).

-Kees

-- 
Kees Cook

Re: [PATCH 2/3] fortify: cosmetic cleanups to __compiletime_strlen

[Kees Cook]

On Tue, Aug 30, 2022 at 01:53:08PM -0700, Nick Desaulniers wrote:
> Two things I noticed in __compiletime_strlen:

Four? :)

> 1. A temporary, __p, is created+used to avoid repeated side effects from
>    multiple evaluation of the macro parameter, but the macro parameter
>    was being used accidentally in __builtin_object_size.

__builtin_object_size(), like sizeof() but unlike __builtin_strlen(),
will not evaluate side-effects: https://godbolt.org/z/Yaa1z7YvK
And using bos on __p will sometimes mask the actual object, so p needs to
stay the argument.

> 2. The temporary has a curious signedness and const-less qualification.
>    Just use __auto_type.

__auto_type is pretty rare in the kernel, but does provide the removal
of "const". Even though the kernel builds with -Wno-pointer-sign, the
explicit case does fix a potential warnings about signedness differences,
not just const differences, for __builtin_strlen() which requires "const
char *", but many arguments are "unsigned char *", "u8 *", etc.

Is __auto_type more readable than the explicit cast? It does seem to
work fine.

> 3. (size_t)-1 is perhaps more readable as -1UL.

That's true, though I kind of prefer (size_t)-1, though yes, it appears
to be the extreme minority in the kernel.

> 4. __p_size == -1UL when __builtin_object_size can't evaluate the
>    object size at compile time. We could just reuse __ret and use one
>    less variable here.

This seems to get entire optimized away by the compiler? I think it's
more readable to keep the explicit variable.

-Kees

> 
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  include/linux/fortify-string.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index c5adad596a3f..aaf73575050f 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -22,11 +22,10 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("
>  
>  #define __compiletime_strlen(p)					\
>  ({								\
> -	unsigned char *__p = (unsigned char *)(p);		\
> -	size_t __ret = (size_t)-1;				\
> -	size_t __p_size = __object_size(p, 1);			\
> -	if (__p_size != (size_t)-1) {				\
> -		size_t __p_len = __p_size - 1;			\
> +	__auto_type __p = (p);					\
> +	size_t __ret = __object_size(__p, 1);			\
> +	if (__ret != -1UL) {					\
> +		size_t __p_len = __ret - 1;			\
>  		if (__builtin_constant_p(__p[__p_len]) &&	\
>  		    __p[__p_len] == '\0')			\
>  			__ret = __builtin_strlen(__p);		\
> -- 
> 2.37.2.672.g94769d06f0-goog
> 

-- 
Kees Cook