From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because __rpm_get_callback() uses offsetof() to compute the address of
the callback in question in struct dev_pm_ops, randomizing the layout
of the latter leads to interesting, but unfortunately also undesirable
results in some cases.

Prevent that from happening by using the __no_randomize_layout
annotation on struct dev_pm_ops.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/pm.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -307,7 +307,7 @@ struct dev_pm_ops {
 	int (*runtime_suspend)(struct device *dev);
 	int (*runtime_resume)(struct device *dev);
 	int (*runtime_idle)(struct device *dev);
-};
+} __no_randomize_layout;
 
 #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 	.suspend = pm_sleep_ptr(suspend_fn), \

On August 4, 2022 10:15:08 AM PDT, "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
>From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
>Because __rpm_get_callback() uses offsetof() to compute the address of
>the callback in question in struct dev_pm_ops, randomizing the layout
>of the latter leads to interesting, but unfortunately also undesirable
>results in some cases.

How does this manifest? This is a compile-time randomization, so offsetof() will find the correct location. Is struct dev_pm_ops created or consumed externally from the kernel at any point?

-Kees

>
>Prevent that from happening by using the __no_randomize_layout
>annotation on struct dev_pm_ops.
>
>Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>---
> include/linux/pm.h |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>Index: linux-pm/include/linux/pm.h
>===================================================================
>--- linux-pm.orig/include/linux/pm.h
>+++ linux-pm/include/linux/pm.h
>@@ -307,7 +307,7 @@ struct dev_pm_ops {
> 	int (*runtime_suspend)(struct device *dev);
> 	int (*runtime_resume)(struct device *dev);
> 	int (*runtime_idle)(struct device *dev);
>-};
>+} __no_randomize_layout;
> 
> #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> 	.suspend = pm_sleep_ptr(suspend_fn), \
>
>
>

-- 
Kees Cook

On Fri, Aug 5, 2022 at 4:12 AM Kees Cook <keescook@chromium.org> wrote:
>
>
>
> On August 4, 2022 10:15:08 AM PDT, "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> >From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> >Because __rpm_get_callback() uses offsetof() to compute the address of
> >the callback in question in struct dev_pm_ops, randomizing the layout
> >of the latter leads to interesting, but unfortunately also undesirable
> >results in some cases.
>
> How does this manifest? This is a compile-time randomization, so offsetof() will find the correct location.

Well, I would think so.

> Is struct dev_pm_ops created or consumed externally from the kernel at any point?

I'm not sure TBH.  I have seen a trace where pci_pm_resume_noirq() is
evidently called via rpm_callback() which should never happen if the
offset computation is correct.

The driver in question (which is out of the tree for now) is modular,
so in theory it could be built separately from the rest of the kernel,
but I think that this still should work, shouldn't it?

> >
> >Prevent that from happening by using the __no_randomize_layout
> >annotation on struct dev_pm_ops.
> >
> >Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >---
> > include/linux/pm.h |    2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >Index: linux-pm/include/linux/pm.h
> >===================================================================
> >--- linux-pm.orig/include/linux/pm.h
> >+++ linux-pm/include/linux/pm.h
> >@@ -307,7 +307,7 @@ struct dev_pm_ops {
> >       int (*runtime_suspend)(struct device *dev);
> >       int (*runtime_resume)(struct device *dev);
> >       int (*runtime_idle)(struct device *dev);
> >-};
> >+} __no_randomize_layout;
> >
> > #define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> >       .suspend = pm_sleep_ptr(suspend_fn), \
> >
> >
> >
>
> --
> Kees Cook

On Fri, Aug 05, 2022 at 04:10:29PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 5, 2022 at 4:12 AM Kees Cook <keescook@chromium.org> wrote:
> >
> >
> >
> > On August 4, 2022 10:15:08 AM PDT, "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > >From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > >Because __rpm_get_callback() uses offsetof() to compute the address of
> > >the callback in question in struct dev_pm_ops, randomizing the layout
> > >of the latter leads to interesting, but unfortunately also undesirable
> > >results in some cases.
> >
> > How does this manifest? This is a compile-time randomization, so offsetof() will find the correct location.
> 
> Well, I would think so.
> 
> > Is struct dev_pm_ops created or consumed externally from the kernel at any point?
> 
> I'm not sure TBH.  I have seen a trace where pci_pm_resume_noirq() is
> evidently called via rpm_callback() which should never happen if the
> offset computation is correct.
> 
> The driver in question (which is out of the tree for now) is modular,

I'm not a fan of making core kernel changes for out of tree modules, but
that said, there is clearly a bug somewhere that I'd like to help solve.

> so in theory it could be built separately from the rest of the kernel,
> but I think that this still should work, shouldn't it?

It should work, yes. This implies something is not working in the build
process, though. Either the external module was built without randstruct
and was somehow allowed to be loaded, or the kernel's randstruct seed was
not present in the module build so a new one was chosen. What do

	modinfo -F vermagic name-of-out-of-tree-module

and

	modinfo -F vermagic some-module-built-with-kernel

show?

-- 
Kees Cook