Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

[Christoph Hellwig]

On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
> Can you send the patch to https://github.com/acpica/acpica ? My change
> was finally accepted, so this whole issue will go away on the next
> refresh. Until then, I don't want to block the entire automatic
> structure selection logic of randstruct on a three-function table. :)

I do not have a github account and no such thing is required for kernel
development.

I've already CCed the ACPI maintainer the last time I sent the patch,
and I would much prefer if they'd just include it to play silly games.
Ccing them and Linus once again to sort this process mess out.

> Given that it's a tiny exclusion for randstruct, and there is already
> a path in motion to fix it, I think this patch is trivial and
> sufficient.

But it's pointless - just do the right thing.

---
>From e8046f6507c2ed60bc501a0c0caa5a3f15f5e3e4 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Sun, 28 May 2017 09:53:45 +0300
Subject: acpi: get rid of acpi_sleep_dispatch

No need for the array of structs of function pointers when we can just
call the handfull of functions directly.

This could be further cleaned up if acpi_gbl_reduced_hardware was defined
true in the ACPI_REDUCED_HARDWARE case, but that's material for the next
round.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/acpi/acpica/hwxfsleep.c | 89 +++++++++--------------------------------
 include/acpi/actypes.h          |  9 -----
 2 files changed, 18 insertions(+), 80 deletions(-)

diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
index 5733b1167e46..66fa3ebddd57 100644
--- a/drivers/acpi/acpica/hwxfsleep.c
+++ b/drivers/acpi/acpica/hwxfsleep.c
@@ -57,26 +57,6 @@ acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs,
 				   acpi_physical_address physical_address64);
 #endif
 
-static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
-
-/*
- * Dispatch table used to efficiently branch to the various sleep
- * functions.
- */
-#define ACPI_SLEEP_FUNCTION_ID         0
-#define ACPI_WAKE_PREP_FUNCTION_ID     1
-#define ACPI_WAKE_FUNCTION_ID          2
-
-/* Legacy functions are optional, based upon ACPI_REDUCED_HARDWARE */
-
-static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
-	 acpi_hw_extended_sleep},
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
-	 acpi_hw_extended_wake_prep},
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake), acpi_hw_extended_wake}
-};
-
 /*
  * These functions are removed for the ACPI_REDUCED_HARDWARE case:
  *      acpi_set_firmware_waking_vector
@@ -236,53 +216,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
 
 ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_s4bios)
 #endif				/* !ACPI_REDUCED_HARDWARE */
-/*******************************************************************************
- *
- * FUNCTION:    acpi_hw_sleep_dispatch
- *
- * PARAMETERS:  sleep_state         - Which sleep state to enter/exit
- *              function_id         - Sleep, wake_prep, or Wake
- *
- * RETURN:      Status from the invoked sleep handling function.
- *
- * DESCRIPTION: Dispatch a sleep/wake request to the appropriate handling
- *              function.
- *
- ******************************************************************************/
-static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id)
-{
-	acpi_status status;
-	struct acpi_sleep_functions *sleep_functions =
-	    &acpi_sleep_dispatch[function_id];
-
-#if (!ACPI_REDUCED_HARDWARE)
-	/*
-	 * If the Hardware Reduced flag is set (from the FADT), we must
-	 * use the extended sleep registers (FADT). Note: As per the ACPI
-	 * specification, these extended registers are to be used for HW-reduced
-	 * platforms only. They are not general-purpose replacements for the
-	 * legacy PM register sleep support.
-	 */
-	if (acpi_gbl_reduced_hardware) {
-		status = sleep_functions->extended_function(sleep_state);
-	} else {
-		/* Legacy sleep */
-
-		status = sleep_functions->legacy_function(sleep_state);
-	}
-
-	return (status);
-
-#else
-	/*
-	 * For the case where reduced-hardware-only code is being generated,
-	 * we know that only the extended sleep registers are available
-	 */
-	status = sleep_functions->extended_function(sleep_state);
-	return (status);
-
-#endif				/* !ACPI_REDUCED_HARDWARE */
-}
 
 /*******************************************************************************
  *
@@ -389,7 +322,12 @@ acpi_status acpi_enter_sleep_state(u8 sleep_state)
 		return_ACPI_STATUS(AE_AML_OPERAND_VALUE);
 	}
 
-	status = acpi_hw_sleep_dispatch(sleep_state, ACPI_SLEEP_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_sleep(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_sleep(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
@@ -415,8 +353,12 @@ acpi_status acpi_leave_sleep_state_prep(u8 sleep_state)
 
 	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state_prep);
 
-	status =
-	    acpi_hw_sleep_dispatch(sleep_state, ACPI_WAKE_PREP_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_wake_prep(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_wake_prep(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
@@ -440,7 +382,12 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
 
 	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state);
 
-	status = acpi_hw_sleep_dispatch(sleep_state, ACPI_WAKE_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_wake(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_wake(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index d549e31c6d18..cfcb3abc65b9 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -894,15 +894,6 @@ typedef u8 acpi_adr_space_type;
 #define ACPI_ENABLE_EVENT                       1
 #define ACPI_DISABLE_EVENT                      0
 
-/* Sleep function dispatch */
-
-typedef acpi_status (*acpi_sleep_function) (u8 sleep_state);
-
-struct acpi_sleep_functions {
-	acpi_sleep_function legacy_function;
-	acpi_sleep_function extended_function;
-};
-
 /*
  * External ACPI object definition
  */
-- 
2.11.0

Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

[Rafael J. Wysocki]

On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>> Can you send the patch to https://github.com/acpica/acpica ? My change
>> was finally accepted, so this whole issue will go away on the next
>> refresh. Until then, I don't want to block the entire automatic
>> structure selection logic of randstruct on a three-function table. :)
>
> I do not have a github account and no such thing is required for kernel
> development.
>
> I've already CCed the ACPI maintainer the last time I sent the patch,
> and I would much prefer if they'd just include it to play silly games.
> Ccing them and Linus once again to sort this process mess out.
>
>> Given that it's a tiny exclusion for randstruct, and there is already
>> a path in motion to fix it, I think this patch is trivial and
>> sufficient.
>
> But it's pointless - just do the right thing.
>
> ---
> From e8046f6507c2ed60bc501a0c0caa5a3f15f5e3e4 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Sun, 28 May 2017 09:53:45 +0300
> Subject: acpi: get rid of acpi_sleep_dispatch
>
> No need for the array of structs of function pointers when we can just
> call the handfull of functions directly.
>
> This could be further cleaned up if acpi_gbl_reduced_hardware was defined
> true in the ACPI_REDUCED_HARDWARE case, but that's material for the next
> round.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/acpi/acpica/hwxfsleep.c | 89 +++++++++--------------------------------
>  include/acpi/actypes.h          |  9 -----
>  2 files changed, 18 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
> index 5733b1167e46..66fa3ebddd57 100644
> --- a/drivers/acpi/acpica/hwxfsleep.c
> +++ b/drivers/acpi/acpica/hwxfsleep.c
> @@ -57,26 +57,6 @@ acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs,
>                                    acpi_physical_address physical_address64);
>  #endif
>
> -static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
> -
> -/*
> - * Dispatch table used to efficiently branch to the various sleep
> - * functions.
> - */
> -#define ACPI_SLEEP_FUNCTION_ID         0
> -#define ACPI_WAKE_PREP_FUNCTION_ID     1
> -#define ACPI_WAKE_FUNCTION_ID          2
> -
> -/* Legacy functions are optional, based upon ACPI_REDUCED_HARDWARE */
> -
> -static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
> -       {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
> -        acpi_hw_extended_sleep},
> -       {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
> -        acpi_hw_extended_wake_prep},
> -       {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake), acpi_hw_extended_wake}
> -};
> -
>  /*
>   * These functions are removed for the ACPI_REDUCED_HARDWARE case:
>   *      acpi_set_firmware_waking_vector
> @@ -236,53 +216,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
>
>  ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_s4bios)
>  #endif                         /* !ACPI_REDUCED_HARDWARE */
> -/*******************************************************************************
> - *
> - * FUNCTION:    acpi_hw_sleep_dispatch
> - *
> - * PARAMETERS:  sleep_state         - Which sleep state to enter/exit
> - *              function_id         - Sleep, wake_prep, or Wake
> - *
> - * RETURN:      Status from the invoked sleep handling function.
> - *
> - * DESCRIPTION: Dispatch a sleep/wake request to the appropriate handling
> - *              function.
> - *
> - ******************************************************************************/
> -static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id)
> -{
> -       acpi_status status;
> -       struct acpi_sleep_functions *sleep_functions =
> -           &acpi_sleep_dispatch[function_id];
> -
> -#if (!ACPI_REDUCED_HARDWARE)
> -       /*
> -        * If the Hardware Reduced flag is set (from the FADT), we must
> -        * use the extended sleep registers (FADT). Note: As per the ACPI
> -        * specification, these extended registers are to be used for HW-reduced
> -        * platforms only. They are not general-purpose replacements for the
> -        * legacy PM register sleep support.
> -        */
> -       if (acpi_gbl_reduced_hardware) {
> -               status = sleep_functions->extended_function(sleep_state);
> -       } else {
> -               /* Legacy sleep */
> -
> -               status = sleep_functions->legacy_function(sleep_state);
> -       }
> -
> -       return (status);
> -
> -#else
> -       /*
> -        * For the case where reduced-hardware-only code is being generated,
> -        * we know that only the extended sleep registers are available
> -        */
> -       status = sleep_functions->extended_function(sleep_state);
> -       return (status);
> -
> -#endif                         /* !ACPI_REDUCED_HARDWARE */
> -}
>
>  /*******************************************************************************
>   *
> @@ -389,7 +322,12 @@ acpi_status acpi_enter_sleep_state(u8 sleep_state)
>                 return_ACPI_STATUS(AE_AML_OPERAND_VALUE);
>         }
>
> -       status = acpi_hw_sleep_dispatch(sleep_state, ACPI_SLEEP_FUNCTION_ID);
> +#if !ACPI_REDUCED_HARDWARE
> +       if (!acpi_gbl_reduced_hardware)
> +               status = acpi_hw_legacy_sleep(sleep_state);
> +       else
> +#endif
> +               status = acpi_hw_extended_sleep(sleep_state);
>         return_ACPI_STATUS(status);
>  }
>

I guess you can avoid these #ifs in function bodies?

Thanks,
Rafael

Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

[Rafael J. Wysocki]

On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>> Can you send the patch to https://github.com/acpica/acpica ? My change
>> was finally accepted, so this whole issue will go away on the next
>> refresh. Until then, I don't want to block the entire automatic
>> structure selection logic of randstruct on a three-function table. :)
>
> I do not have a github account and no such thing is required for kernel
> development.

It isn't required for the ACPICA material either.

You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
your ACPICA patches.  They pick up stuff that looks good to them.

And we tend to prefer routing ACPICA changes through the upstream,
because failing to do so usually turns out to be painful in the long
term.  I don't think it is unreasonable to ask for cooperation in that
respect.

Thanks,
Rafael

Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

[Kees Cook]

On Tue, Jun 20, 2017 at 2:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>>> Can you send the patch to https://github.com/acpica/acpica ? My change
>>> was finally accepted, so this whole issue will go away on the next
>>> refresh. Until then, I don't want to block the entire automatic
>>> structure selection logic of randstruct on a three-function table. :)
>>
>> I do not have a github account and no such thing is required for kernel
>> development.
>
> It isn't required for the ACPICA material either.
>
> You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
> your ACPICA patches.  They pick up stuff that looks good to them.
>
> And we tend to prefer routing ACPICA changes through the upstream,
> because failing to do so usually turns out to be painful in the long
> term.  I don't think it is unreasonable to ask for cooperation in that
> respect.

I'd like to unblock randstruct, so what's the easiest way to move
this? My version of changes have already landed upstream in ACPICA,
but I don't know how frequently they get flushed back into the kernel.

I can't turn on randstruct auto-selection in -next without either
ACPICA using (or not needing) designated initializers or just
blacklisting it in the randstruct plugin itself. I would much prefer
the latter as the problem is solved in ACPICA upstream already but
just isn't in the kernel yet, and I want to get testing of the
auto-selection ASAP. Once it's in the kernel I can drop the blacklist.

Christoph: how about a middle ground where randstruct blacklists
ACPICA in -next and if ACPICA is fixed by the time the merge window
opens, I'll drop the blacklist. That gets the testing coverage without
what you see as an ugly hack right now. I just really don't want to
waste any more time on this since there are SO many other randomized
structures I'd like to be sure are getting testing.

Alternatively, if the ACPICA folks Ack Christoph's patch, I can carry
that in the randstruct tree for -next instead?

Thanks,

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

[Rafael J. Wysocki]

On Thursday, June 22, 2017 04:57:39 PM Kees Cook wrote:
> On Tue, Jun 20, 2017 at 2:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
> >>> Can you send the patch to https://github.com/acpica/acpica ? My change
> >>> was finally accepted, so this whole issue will go away on the next
> >>> refresh. Until then, I don't want to block the entire automatic
> >>> structure selection logic of randstruct on a three-function table. :)
> >>
> >> I do not have a github account and no such thing is required for kernel
> >> development.
> >
> > It isn't required for the ACPICA material either.
> >
> > You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
> > your ACPICA patches.  They pick up stuff that looks good to them.
> >
> > And we tend to prefer routing ACPICA changes through the upstream,
> > because failing to do so usually turns out to be painful in the long
> > term.  I don't think it is unreasonable to ask for cooperation in that
> > respect.
> 
> I'd like to unblock randstruct, so what's the easiest way to move
> this? My version of changes have already landed upstream in ACPICA,
> but I don't know how frequently they get flushed back into the kernel.

Usually, when there's a new ACPICA release, but occasionally that happens
faster.

Which commit in upstream ACPICA is this?

> I can't turn on randstruct auto-selection in -next without either
> ACPICA using (or not needing) designated initializers or just
> blacklisting it in the randstruct plugin itself. I would much prefer
> the latter as the problem is solved in ACPICA upstream already but
> just isn't in the kernel yet, and I want to get testing of the
> auto-selection ASAP. Once it's in the kernel I can drop the blacklist.
> 
> Christoph: how about a middle ground where randstruct blacklists
> ACPICA in -next and if ACPICA is fixed by the time the merge window
> opens, I'll drop the blacklist. That gets the testing coverage without
> what you see as an ugly hack right now. I just really don't want to
> waste any more time on this since there are SO many other randomized
> structures I'd like to be sure are getting testing.
> 
> Alternatively, if the ACPICA folks Ack Christoph's patch, I can carry
> that in the randstruct tree for -next instead?

Maybe we can simply forward port the ACPICA commit right away.

Lv, can you take care of this, please?

Thanks,
Rafael

Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

[Kees Cook]

On Thu, Jun 22, 2017 at 4:59 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 04:57:39 PM Kees Cook wrote:
>> On Tue, Jun 20, 2017 at 2:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> >> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>> >>> Can you send the patch to https://github.com/acpica/acpica ? My change
>> >>> was finally accepted, so this whole issue will go away on the next
>> >>> refresh. Until then, I don't want to block the entire automatic
>> >>> structure selection logic of randstruct on a three-function table. :)
>> >>
>> >> I do not have a github account and no such thing is required for kernel
>> >> development.
>> >
>> > It isn't required for the ACPICA material either.
>> >
>> > You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
>> > your ACPICA patches.  They pick up stuff that looks good to them.
>> >
>> > And we tend to prefer routing ACPICA changes through the upstream,
>> > because failing to do so usually turns out to be painful in the long
>> > term.  I don't think it is unreasonable to ask for cooperation in that
>> > respect.
>>
>> I'd like to unblock randstruct, so what's the easiest way to move
>> this? My version of changes have already landed upstream in ACPICA,
>> but I don't know how frequently they get flushed back into the kernel.
>
> Usually, when there's a new ACPICA release, but occasionally that happens
> faster.
>
> Which commit in upstream ACPICA is this?

https://github.com/acpica/acpica/commit/2058b3bf5deecb9644d676703bd97d1bce5e612a

>> I can't turn on randstruct auto-selection in -next without either
>> ACPICA using (or not needing) designated initializers or just
>> blacklisting it in the randstruct plugin itself. I would much prefer
>> the latter as the problem is solved in ACPICA upstream already but
>> just isn't in the kernel yet, and I want to get testing of the
>> auto-selection ASAP. Once it's in the kernel I can drop the blacklist.
>>
>> Christoph: how about a middle ground where randstruct blacklists
>> ACPICA in -next and if ACPICA is fixed by the time the merge window
>> opens, I'll drop the blacklist. That gets the testing coverage without
>> what you see as an ugly hack right now. I just really don't want to
>> waste any more time on this since there are SO many other randomized
>> structures I'd like to be sure are getting testing.
>>
>> Alternatively, if the ACPICA folks Ack Christoph's patch, I can carry
>> that in the randstruct tree for -next instead?
>
> Maybe we can simply forward port the ACPICA commit right away.
>
> Lv, can you take care of this, please?

That would be great, thanks!

-Kees

-- 
Kees Cook
Pixel Security