Re: [PATCH] Add TAINT_DRIVER_OVERRIDE for usage of driver_override

[Armin Wolf]

Am 25.09.26 um 20:20 schrieb Uwe Kleine-König:

Commit fcbfaffee51a ("driver core: add TAINT_FORCED_BIND for when
userspace manually messes with devices and drivers") introduced a taint
for usage of bind/unbind sysfs files that manually trigger driver probe
and remove respectively.

For drivers that do their resource management correctly (which is also
needed for module unloading) bind and unbind for matching devices are
not critical operations. The thing that makes bind and unbind unsafe is
that drivers can be forced on devices that originally don't match using
driver_override. The result is that e.g. of_device_get_match_data()
returns NULL despite all .of_match_table entries having a non-NULL
.driver_data member which yields a NULL pointer exception for several
drivers. And given that after setting a driver_override a manual bind is
only one way a driver can be bound to an unexpected device, a separate
taint for such an override is justified.

Signed-off-by: Uwe Kleine-König <u.kleine-koe...@baylibre.com>
---
Hello,

I first considered to just set TAINT_FORCED_BIND when driver_override is
written to. But IMHO it's very useful to differentiate for reported bugs
if bind/unbind was used in combination with driver_override or not.

Also if only bind/unbind was used and driver_override not, a resulting
problem is worth fixing (opposed to what is considered for
bind+driver_override).

IMHO it would also be fine to drop TAINT_FORCED_BIND, but let time prove
how many reports we get with TAINT_FORCED_BIND but without
TAINT_DRIVER_OVERRIDE.

Best regards
Uwe

Good idea, lets see if TAINT_FORCED_BIND is really necessary after all.

Reviewed-by: Armin Wolf <w_ar...@gmx.de>

  Documentation/admin-guide/tainted-kernels.rst | 6 +++++-
  drivers/base/bus.c                            | 1 +
  include/linux/panic.h                         | 3 ++-
  include/trace/events/module.h                 | 3 ++-
  kernel/panic.c                                | 3 ++-
  tools/debugging/kernel-chktaint               | 8 ++++++++
  6 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst 
b/Documentation/admin-guide/tainted-kernels.rst
index abbf5e3dd749..75750a9533e9 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -74,7 +74,7 @@ a particular type of taint. It's best to leave that to the 
aforementioned
  script, but if you need something quick you can use this shell command to 
check
  which bits are set::

- $ for i in $(seq 20); do echo $(($i-1)) $(($(cat /proc/sys/kernel/tainted)>>($i-1)&1));done

+       $ for i in $(seq 21); do echo $(($i-1)) $(($(cat 
/proc/sys/kernel/tainted)>>($i-1)&1));done

Table for decoding tainted state

  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -103,6 +103,7 @@ Bit  Log  Number   Reason that got the kernel tainted
   18  _/N   262144  an in-kernel test has been run
   19  _/J   524288  userspace used a mutating debug operation in fwctl
   20  _/Y  1048576  device was manually bound or unbound from a driver
+ 21  _/Z  2097152  a driver was forced on a non-matching device
  ===  ===  =======  ========================================================

Note: The character ``_`` is representing a blank in this table to make reading

@@ -193,3 +194,6 @@ More detailed explanation for tainting

20) ``Y`` If userspace wrote to the `bind` or `unbind` sysfs files and

       successfully bound or removed a device from a driver.
+
+ 21) ``Z`` If userspace wrote to a `driver_override` sysfs file opening the 
gate
+     for unexpected driver binding.
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index c51ad96d4de4..6e91210aab45 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -513,6 +513,7 @@ static ssize_t driver_override_store(struct device *dev,
  {
        int ret;

+ add_taint_module(drv->owner, TAINT_DRIVER_OVERRIDE, LOCKDEP_STILL_OK);

        ret = __device_set_driver_override(dev, buf, count);
        if (ret)
                return ret;
diff --git a/include/linux/panic.h b/include/linux/panic.h
index 23976b1dfdb6..e6e24d8afcf7 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -90,7 +90,8 @@ static inline void set_arch_panic_timeout(int timeout, int 
arch_default_timeout)
  #define TAINT_TEST                    18
  #define TAINT_FWCTL                   19
  #define TAINT_FORCED_BIND             20
-#define TAINT_FLAGS_COUNT              21
+#define TAINT_DRIVER_OVERRIDE          21
+#define TAINT_FLAGS_COUNT              22
  #define TAINT_FLAGS_MAX                       ((1UL << TAINT_FLAGS_COUNT) - 1)

struct taint_flag {

diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 19df3e39bba4..c7cdb1f53bc6 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -27,7 +27,8 @@ struct module;
        { (1UL << TAINT_FORCED_MODULE),           "F" },                \
        { (1UL << TAINT_CRAP),                    "C" },                \
        { (1UL << TAINT_UNSIGNED_MODULE), "E" },                \
-       { (1UL << TAINT_FORCED_BIND),             "Y" })
+       { (1UL << TAINT_FORCED_BIND),             "Y" },                \
+       { (1UL << TAINT_DRIVER_OVERRIDE), "Z" })

TRACE_EVENT(module_load, diff --git a/kernel/panic.c b/kernel/panic.c

index b824b68fcb08..f5476a61f6f3 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -826,6 +826,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
        TAINT_FLAG(TEST,                        'N', ' '),
        TAINT_FLAG(FWCTL,                       'J', ' '),
        TAINT_FLAG(FORCED_BIND,                 'Y', ' '),
+       TAINT_FLAG(DRIVER_OVERRIDE,             'Z', ' '),
  };

#undef TAINT_FLAG

@@ -862,7 +863,7 @@ static void print_tainted_seq(struct seq_buf *s, bool 
verbose)
   * exact size is allocated dynamically; the initial buffer remains
   * as a fallback if allocation fails.
   *
- * The verbose taint string currently requires up to 344 characters.
+ * The verbose taint string currently requires up to 364 characters.
   */
  #define INIT_TAINT_BUF_MAX 370

diff --git a/tools/debugging/kernel-chktaint b/tools/debugging/kernel-chktaint

index d8628be37214..14d8febd6b16 100755
--- a/tools/debugging/kernel-chktaint
+++ b/tools/debugging/kernel-chktaint
@@ -219,6 +219,14 @@ else
        addout "Y"
        echo " * device was manually bound or unbound from a driver (#20)"
  fi
+
+T=`expr $T / 2`
+if [ `expr $T % 2` -eq 0 ]; then
+       addout " "
+else
+       addout "Z"
+       echo " * a driver was forced on a non-matching device (#21)"
+fi
  echo "Raw taint value as int/string: $taint/'$out'"

# report on any tainted loadable modules