[PATCH 0/2] Fix debugfs register access while suspended

[PATCH 0/2] Fix debugfs register access while suspended

[Geert Uytterhoeven]

	Hi all,

While comparing register values read from debugfs files under
/sys/kernel/debug/ccree/, I noticed some oddities.
Apparently there is no guarantee these registers are read from the
device while it is resumed.  This may lead to bogus values, or crashes
and lock-ups.

This patch series:
  1. Allows debugfs_create_regset32() to be used for devices whose
     registers must be accessed when resumed,
  2. Fixes the CCREE driver to make use of this.

I have identified several other drivers that may be affected (i.e.
using debugfs_create_regset32() and pm_runtime_*()):
  - drivers/gpu/drm/msm/disp/dpu1
  - drivers/usb/dwc3
  - drivers/usb/host/ehci-omap.c
  - drivers/usb/host/ehci-tegra.c
  - drivers/usb/host/ohci-platform.c
  - drivers/usb/host/xhci.c
  - drivers/usb/host/xhci-dbgcap.c
  - drivers/usb/host/xhci-histb.c
  - drivers/usb/host/xhci-hub.c
  - drivers/usb/host/xhci-mtk.c
  - drivers/usb/host/xhci-pci.c
  - drivers/usb/host/xhci-plat.c
  - drivers/usb/host/xhci-tegra.c
  - drivers/usb/mtu3
  - drivers/usb/musb

Some of these call pm_runtime_forbid(), but given the comment "users
should enable runtime pm using power/control in sysfs", this can be
overridden from userspace, so these are unsafe, too?

Thanks for your comments!

Geert Uytterhoeven (2):
  debugfs: regset32: Add Runtime PM support
  crypto: ccree - fix debugfs register access while suspended

 drivers/crypto/ccree/cc_debugfs.c | 2 ++
 fs/debugfs/file.c                 | 8 ++++++++
 include/linux/debugfs.h           | 1 +
 3 files changed, 11 insertions(+)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/2] debugfs: regset32: Add Runtime PM support

[Geert Uytterhoeven]

Hardware registers of devices under control of power management cannot
be accessed at all times.  If such a device is suspended, register
accesses may lead to undefined behavior, like reading bogus values, or
causing exceptions or system locks.

Extend struct debugfs_regset32 with an optional field to let device
drivers specify the device the registers in the set belong to.  This
allows debugfs_show_regset32() to make sure the device is resumed while
its registers are being read.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 fs/debugfs/file.c       | 8 ++++++++
 include/linux/debugfs.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index dede25247b81f72a..5e52d68421c678f2 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <linux/device.h>
+#include <linux/pm_runtime.h>
 #include <linux/poll.h>
 #include <linux/security.h>
 
@@ -1057,7 +1058,14 @@ static int debugfs_show_regset32(struct seq_file *s, void *data)
 {
 	struct debugfs_regset32 *regset = s->private;
 
+	if (regset->dev)
+		pm_runtime_get_sync(regset->dev);
+
 	debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
+
+	if (regset->dev)
+		pm_runtime_put(regset->dev);
+
 	return 0;
 }
 
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index bf9b6cafa4c26a68..5d0783ae09f365ac 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -35,6 +35,7 @@ struct debugfs_regset32 {
 	const struct debugfs_reg32 *regs;
 	int nregs;
 	void __iomem *base;
+	struct device *dev;	/* Optional device for Runtime PM */
 };
 
 extern struct dentry *arch_debugfs_dir;
-- 
2.17.1

[PATCH 2/2] crypto: ccree - fix debugfs register access while suspended

[Geert Uytterhoeven]

Reading the debugfs files under /sys/kernel/debug/ccree/ can be done by
the user at any time.  On R-Car SoCs, the CCREE device is power-managed
using a moduile clock, and if this clock is not running, bogus register
values may be read.

Fix this by filling in the debugfs_regset32.dev field, so debugfs will
make sure the device is resumed while its registers are being read.

This fixes the bogus values (0x00000260) in the register dumps on R-Car
H3 ES1.0:

    -e6601000.crypto/regs:HOST_IRR = 0x00000260
    -e6601000.crypto/regs:HOST_POWER_DOWN_EN = 0x00000260
    +e6601000.crypto/regs:HOST_IRR = 0x00000038
    +e6601000.crypto/regs:HOST_POWER_DOWN_EN = 0x00000038
     e6601000.crypto/regs:AXIM_MON_ERR = 0x00000000
     e6601000.crypto/regs:DSCRPTR_QUEUE_CONTENT = 0x000002aa
    -e6601000.crypto/regs:HOST_IMR = 0x00000260
    +e6601000.crypto/regs:HOST_IMR = 0x017ffeff
     e6601000.crypto/regs:AXIM_CFG = 0x001f0007
     e6601000.crypto/regs:AXIM_CACHE_PARAMS = 0x00000000
    -e6601000.crypto/regs:GPR_HOST = 0x00000260
    +e6601000.crypto/regs:GPR_HOST = 0x017ffeff
     e6601000.crypto/regs:AXIM_MON_COMP = 0x00000000
    -e6601000.crypto/version:SIGNATURE = 0x00000260
    -e6601000.crypto/version:VERSION = 0x00000260
    +e6601000.crypto/version:SIGNATURE = 0xdcc63000
    +e6601000.crypto/version:VERSION = 0xaf400001

Note that this behavior is system-dependent, and the issue does not show
up on all R-Car Gen3 SoCs and boards.  Even when the device is
suspended, the module clock may be left enabled, if configured by the
firmware for Secure Mode, or when controlled by the Real-Time Core.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/crypto/ccree/cc_debugfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/ccree/cc_debugfs.c b/drivers/crypto/ccree/cc_debugfs.c
index 5669997386988055..35f3a2137502bd96 100644
--- a/drivers/crypto/ccree/cc_debugfs.c
+++ b/drivers/crypto/ccree/cc_debugfs.c
@@ -81,6 +81,7 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
 	regset->regs = debug_regs;
 	regset->nregs = ARRAY_SIZE(debug_regs);
 	regset->base = drvdata->cc_base;
+	regset->dev = dev;
 
 	ctx->dir = debugfs_create_dir(drvdata->plat_dev->name, cc_debugfs_dir);
 
@@ -102,6 +103,7 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
 		verset->nregs = ARRAY_SIZE(pid_cid_regs);
 	}
 	verset->base = drvdata->cc_base;
+	verset->dev = dev;
 
 	debugfs_create_regset32("version", 0400, ctx->dir, verset);
 
-- 
2.17.1

Re: [PATCH 0/2] Fix debugfs register access while suspended

[Niklas Söderlund]

Hi Geert,

Thanks for your series.

On 2020-01-24 14:29:55 +0100, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> While comparing register values read from debugfs files under
> /sys/kernel/debug/ccree/, I noticed some oddities.
> Apparently there is no guarantee these registers are read from the
> device while it is resumed.  This may lead to bogus values, or crashes
> and lock-ups.
> 
> This patch series:
>   1. Allows debugfs_create_regset32() to be used for devices whose
>      registers must be accessed when resumed,
>   2. Fixes the CCREE driver to make use of this.
> 
> I have identified several other drivers that may be affected (i.e.
> using debugfs_create_regset32() and pm_runtime_*()):
>   - drivers/gpu/drm/msm/disp/dpu1
>   - drivers/usb/dwc3
>   - drivers/usb/host/ehci-omap.c
>   - drivers/usb/host/ehci-tegra.c
>   - drivers/usb/host/ohci-platform.c
>   - drivers/usb/host/xhci.c
>   - drivers/usb/host/xhci-dbgcap.c
>   - drivers/usb/host/xhci-histb.c
>   - drivers/usb/host/xhci-hub.c
>   - drivers/usb/host/xhci-mtk.c
>   - drivers/usb/host/xhci-pci.c
>   - drivers/usb/host/xhci-plat.c
>   - drivers/usb/host/xhci-tegra.c
>   - drivers/usb/mtu3
>   - drivers/usb/musb
> 
> Some of these call pm_runtime_forbid(), but given the comment "users
> should enable runtime pm using power/control in sysfs", this can be
> overridden from userspace, so these are unsafe, too?
> 
> Thanks for your comments!

Looks good to me,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> Geert Uytterhoeven (2):
>   debugfs: regset32: Add Runtime PM support
>   crypto: ccree - fix debugfs register access while suspended
> 
>  drivers/crypto/ccree/cc_debugfs.c | 2 ++
>  fs/debugfs/file.c                 | 8 ++++++++
>  include/linux/debugfs.h           | 1 +
>  3 files changed, 11 insertions(+)
> 
> -- 
> 2.17.1
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> 							    -- Linus Torvalds

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/2] crypto: ccree - fix debugfs register access while suspended

[Gilad Ben-Yossef]

On Fri, Jan 24, 2020 at 3:30 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Reading the debugfs files under /sys/kernel/debug/ccree/ can be done by
> the user at any time.  On R-Car SoCs, the CCREE device is power-managed
> using a moduile clock, and if this clock is not running, bogus register
> values may be read.
>
> Fix this by filling in the debugfs_regset32.dev field, so debugfs will
> make sure the device is resumed while its registers are being read.
>
> This fixes the bogus values (0x00000260) in the register dumps on R-Car
> H3 ES1.0:
>
>     -e6601000.crypto/regs:HOST_IRR = 0x00000260
>     -e6601000.crypto/regs:HOST_POWER_DOWN_EN = 0x00000260
>     +e6601000.crypto/regs:HOST_IRR = 0x00000038
>     +e6601000.crypto/regs:HOST_POWER_DOWN_EN = 0x00000038
>      e6601000.crypto/regs:AXIM_MON_ERR = 0x00000000
>      e6601000.crypto/regs:DSCRPTR_QUEUE_CONTENT = 0x000002aa
>     -e6601000.crypto/regs:HOST_IMR = 0x00000260
>     +e6601000.crypto/regs:HOST_IMR = 0x017ffeff
>      e6601000.crypto/regs:AXIM_CFG = 0x001f0007
>      e6601000.crypto/regs:AXIM_CACHE_PARAMS = 0x00000000
>     -e6601000.crypto/regs:GPR_HOST = 0x00000260
>     +e6601000.crypto/regs:GPR_HOST = 0x017ffeff
>      e6601000.crypto/regs:AXIM_MON_COMP = 0x00000000
>     -e6601000.crypto/version:SIGNATURE = 0x00000260
>     -e6601000.crypto/version:VERSION = 0x00000260
>     +e6601000.crypto/version:SIGNATURE = 0xdcc63000
>     +e6601000.crypto/version:VERSION = 0xaf400001
>
> Note that this behavior is system-dependent, and the issue does not show
> up on all R-Car Gen3 SoCs and boards.  Even when the device is
> suspended, the module clock may be left enabled, if configured by the
> firmware for Secure Mode, or when controlled by the Real-Time Core.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/crypto/ccree/cc_debugfs.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/crypto/ccree/cc_debugfs.c b/drivers/crypto/ccree/cc_debugfs.c
> index 5669997386988055..35f3a2137502bd96 100644
> --- a/drivers/crypto/ccree/cc_debugfs.c
> +++ b/drivers/crypto/ccree/cc_debugfs.c
> @@ -81,6 +81,7 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
>         regset->regs = debug_regs;
>         regset->nregs = ARRAY_SIZE(debug_regs);
>         regset->base = drvdata->cc_base;
> +       regset->dev = dev;
>
>         ctx->dir = debugfs_create_dir(drvdata->plat_dev->name, cc_debugfs_dir);
>
> @@ -102,6 +103,7 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
>                 verset->nregs = ARRAY_SIZE(pid_cid_regs);
>         }
>         verset->base = drvdata->cc_base;
> +       verset->dev = dev;
>
>         debugfs_create_regset32("version", 0400, ctx->dir, verset);
>
> --
> 2.17.1
>


Acked-by: Gilad Ben-Yossef <gilad@benyossef.com>

Thanks,
Gilad
-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Re: [PATCH 2/2] crypto: ccree - fix debugfs register access while suspended

[Rafael J. Wysocki]

On Fri, Jan 24, 2020 at 2:30 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Reading the debugfs files under /sys/kernel/debug/ccree/ can be done by
> the user at any time.  On R-Car SoCs, the CCREE device is power-managed
> using a moduile clock, and if this clock is not running, bogus register
> values may be read.
>
> Fix this by filling in the debugfs_regset32.dev field, so debugfs will
> make sure the device is resumed while its registers are being read.
>
> This fixes the bogus values (0x00000260) in the register dumps on R-Car
> H3 ES1.0:
>
>     -e6601000.crypto/regs:HOST_IRR = 0x00000260
>     -e6601000.crypto/regs:HOST_POWER_DOWN_EN = 0x00000260
>     +e6601000.crypto/regs:HOST_IRR = 0x00000038
>     +e6601000.crypto/regs:HOST_POWER_DOWN_EN = 0x00000038
>      e6601000.crypto/regs:AXIM_MON_ERR = 0x00000000
>      e6601000.crypto/regs:DSCRPTR_QUEUE_CONTENT = 0x000002aa
>     -e6601000.crypto/regs:HOST_IMR = 0x00000260
>     +e6601000.crypto/regs:HOST_IMR = 0x017ffeff
>      e6601000.crypto/regs:AXIM_CFG = 0x001f0007
>      e6601000.crypto/regs:AXIM_CACHE_PARAMS = 0x00000000
>     -e6601000.crypto/regs:GPR_HOST = 0x00000260
>     +e6601000.crypto/regs:GPR_HOST = 0x017ffeff
>      e6601000.crypto/regs:AXIM_MON_COMP = 0x00000000
>     -e6601000.crypto/version:SIGNATURE = 0x00000260
>     -e6601000.crypto/version:VERSION = 0x00000260
>     +e6601000.crypto/version:SIGNATURE = 0xdcc63000
>     +e6601000.crypto/version:VERSION = 0xaf400001
>
> Note that this behavior is system-dependent, and the issue does not show
> up on all R-Car Gen3 SoCs and boards.  Even when the device is
> suspended, the module clock may be left enabled, if configured by the
> firmware for Secure Mode, or when controlled by the Real-Time Core.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

LGTM:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/crypto/ccree/cc_debugfs.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/crypto/ccree/cc_debugfs.c b/drivers/crypto/ccree/cc_debugfs.c
> index 5669997386988055..35f3a2137502bd96 100644
> --- a/drivers/crypto/ccree/cc_debugfs.c
> +++ b/drivers/crypto/ccree/cc_debugfs.c
> @@ -81,6 +81,7 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
>         regset->regs = debug_regs;
>         regset->nregs = ARRAY_SIZE(debug_regs);
>         regset->base = drvdata->cc_base;
> +       regset->dev = dev;
>
>         ctx->dir = debugfs_create_dir(drvdata->plat_dev->name, cc_debugfs_dir);
>
> @@ -102,6 +103,7 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
>                 verset->nregs = ARRAY_SIZE(pid_cid_regs);
>         }
>         verset->base = drvdata->cc_base;
> +       verset->dev = dev;
>
>         debugfs_create_regset32("version", 0400, ctx->dir, verset);
>
> --
> 2.17.1
>

Re: [PATCH 1/2] debugfs: regset32: Add Runtime PM support

[Rafael J. Wysocki]

On Fri, Jan 24, 2020 at 2:30 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Hardware registers of devices under control of power management cannot
> be accessed at all times.  If such a device is suspended, register
> accesses may lead to undefined behavior, like reading bogus values, or
> causing exceptions or system locks.
>
> Extend struct debugfs_regset32 with an optional field to let device
> drivers specify the device the registers in the set belong to.  This
> allows debugfs_show_regset32() to make sure the device is resumed while
> its registers are being read.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

LGTM:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  fs/debugfs/file.c       | 8 ++++++++
>  include/linux/debugfs.h | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index dede25247b81f72a..5e52d68421c678f2 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/atomic.h>
>  #include <linux/device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/poll.h>
>  #include <linux/security.h>
>
> @@ -1057,7 +1058,14 @@ static int debugfs_show_regset32(struct seq_file *s, void *data)
>  {
>         struct debugfs_regset32 *regset = s->private;
>
> +       if (regset->dev)
> +               pm_runtime_get_sync(regset->dev);
> +
>         debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
> +
> +       if (regset->dev)
> +               pm_runtime_put(regset->dev);
> +
>         return 0;
>  }
>
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index bf9b6cafa4c26a68..5d0783ae09f365ac 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -35,6 +35,7 @@ struct debugfs_regset32 {
>         const struct debugfs_reg32 *regs;
>         int nregs;
>         void __iomem *base;
> +       struct device *dev;     /* Optional device for Runtime PM */
>  };
>
>  extern struct dentry *arch_debugfs_dir;
> --
> 2.17.1
>

Re: [PATCH 1/2] debugfs: regset32: Add Runtime PM support

[Greg Kroah-Hartman]

On Fri, Jan 24, 2020 at 02:29:56PM +0100, Geert Uytterhoeven wrote:
> Hardware registers of devices under control of power management cannot
> be accessed at all times.  If such a device is suspended, register
> accesses may lead to undefined behavior, like reading bogus values, or
> causing exceptions or system locks.
> 
> Extend struct debugfs_regset32 with an optional field to let device
> drivers specify the device the registers in the set belong to.  This
> allows debugfs_show_regset32() to make sure the device is resumed while
> its registers are being read.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  fs/debugfs/file.c       | 8 ++++++++
>  include/linux/debugfs.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index dede25247b81f72a..5e52d68421c678f2 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/atomic.h>
>  #include <linux/device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/poll.h>
>  #include <linux/security.h>
>  
> @@ -1057,7 +1058,14 @@ static int debugfs_show_regset32(struct seq_file *s, void *data)
>  {
>  	struct debugfs_regset32 *regset = s->private;
>  
> +	if (regset->dev)
> +		pm_runtime_get_sync(regset->dev);
> +
>  	debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
> +
> +	if (regset->dev)
> +		pm_runtime_put(regset->dev);
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index bf9b6cafa4c26a68..5d0783ae09f365ac 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -35,6 +35,7 @@ struct debugfs_regset32 {
>  	const struct debugfs_reg32 *regs;
>  	int nregs;
>  	void __iomem *base;
> +	struct device *dev;	/* Optional device for Runtime PM */
>  };
>  
>  extern struct dentry *arch_debugfs_dir;

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>