linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86/e820: fix the function type for e820__mapped_all
@ 2020-11-13 18:26 Sami Tolvanen
  2020-11-13 18:34 ` Randy Dunlap
  2020-11-27 12:15 ` Borislav Petkov
  0 siblings, 2 replies; 6+ messages in thread
From: Sami Tolvanen @ 2020-11-13 18:26 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov
  Cc: Kees Cook, x86, linux-kernel, Sami Tolvanen, Sedat Dilek

e820__mapped_all is passed as a callback to is_mmconf_reserved, which
expects a function of type:

  typedef bool (*check_reserved_t)(u64 start, u64 end, unsigned type);

This trips indirect call checking with Clang's Control-Flow Integrity
(CFI). Change the last argument from enum e820_type to unsigned to fix
the type mismatch.

Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/x86/include/asm/e820/api.h | 2 +-
 arch/x86/kernel/e820.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
index e8f58ddd06d9..e872a796619d 100644
--- a/arch/x86/include/asm/e820/api.h
+++ b/arch/x86/include/asm/e820/api.h
@@ -12,7 +12,7 @@ extern unsigned long pci_mem_start;
 
 extern bool e820__mapped_raw_any(u64 start, u64 end, enum e820_type type);
 extern bool e820__mapped_any(u64 start, u64 end, enum e820_type type);
-extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
+extern bool e820__mapped_all(u64 start, u64 end, unsigned type);
 
 extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
 extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 22aad412f965..9f6a4e9bca4c 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -145,7 +145,7 @@ static struct e820_entry *__e820__mapped_all(u64 start, u64 end,
 /*
  * This function checks if the entire range <start,end> is mapped with type.
  */
-bool __init e820__mapped_all(u64 start, u64 end, enum e820_type type)
+bool __init e820__mapped_all(u64 start, u64 end, unsigned type)
 {
 	return __e820__mapped_all(start, end, type);
 }

base-commit: 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
-- 
2.29.2.299.gdc1121823c-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/e820: fix the function type for e820__mapped_all
  2020-11-13 18:26 [PATCH] x86/e820: fix the function type for e820__mapped_all Sami Tolvanen
@ 2020-11-13 18:34 ` Randy Dunlap
  2020-11-13 18:59   ` Sami Tolvanen
  2020-11-27 12:15 ` Borislav Petkov
  1 sibling, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2020-11-13 18:34 UTC (permalink / raw)
  To: Sami Tolvanen, Thomas Gleixner, Ingo Molnar, Borislav Petkov
  Cc: Kees Cook, x86, linux-kernel, Sedat Dilek

On 11/13/20 10:26 AM, Sami Tolvanen wrote:
> e820__mapped_all is passed as a callback to is_mmconf_reserved, which
> expects a function of type:
> 
>   typedef bool (*check_reserved_t)(u64 start, u64 end, unsigned type);
> 
> This trips indirect call checking with Clang's Control-Flow Integrity
> (CFI). Change the last argument from enum e820_type to unsigned to fix
> the type mismatch.

Hi,

Kernel style is no raw unsigned -- use unsigned int or unsigned long, please.

checkpatch should or could have found that issue.

> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/x86/include/asm/e820/api.h | 2 +-
>  arch/x86/kernel/e820.c          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> index e8f58ddd06d9..e872a796619d 100644
> --- a/arch/x86/include/asm/e820/api.h
> +++ b/arch/x86/include/asm/e820/api.h
> @@ -12,7 +12,7 @@ extern unsigned long pci_mem_start;
>  
>  extern bool e820__mapped_raw_any(u64 start, u64 end, enum e820_type type);
>  extern bool e820__mapped_any(u64 start, u64 end, enum e820_type type);
> -extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
> +extern bool e820__mapped_all(u64 start, u64 end, unsigned type);
>  
>  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 22aad412f965..9f6a4e9bca4c 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -145,7 +145,7 @@ static struct e820_entry *__e820__mapped_all(u64 start, u64 end,
>  /*
>   * This function checks if the entire range <start,end> is mapped with type.
>   */
> -bool __init e820__mapped_all(u64 start, u64 end, enum e820_type type)
> +bool __init e820__mapped_all(u64 start, u64 end, unsigned type)
>  {
>  	return __e820__mapped_all(start, end, type);
>  }
> 
> base-commit: 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
> 


-- 
~Randy


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/e820: fix the function type for e820__mapped_all
  2020-11-13 18:34 ` Randy Dunlap
@ 2020-11-13 18:59   ` Sami Tolvanen
  2020-11-13 19:01     ` Randy Dunlap
  0 siblings, 1 reply; 6+ messages in thread
From: Sami Tolvanen @ 2020-11-13 18:59 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Kees Cook, X86 ML,
	LKML, Sedat Dilek

On Fri, Nov 13, 2020 at 10:35 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 11/13/20 10:26 AM, Sami Tolvanen wrote:
> > e820__mapped_all is passed as a callback to is_mmconf_reserved, which
> > expects a function of type:
> >
> >   typedef bool (*check_reserved_t)(u64 start, u64 end, unsigned type);
> >
> > This trips indirect call checking with Clang's Control-Flow Integrity
> > (CFI). Change the last argument from enum e820_type to unsigned to fix
> > the type mismatch.
>
> Hi,
>
> Kernel style is no raw unsigned -- use unsigned int or unsigned long, please.
>
> checkpatch should or could have found that issue.

It did, but the existing type definition for the callback in
pci/mmconfig-shared.c uses raw unsigned. I can add a patch to change
that to unsigned int as well, if you prefer.

Sami

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/e820: fix the function type for e820__mapped_all
  2020-11-13 18:59   ` Sami Tolvanen
@ 2020-11-13 19:01     ` Randy Dunlap
  0 siblings, 0 replies; 6+ messages in thread
From: Randy Dunlap @ 2020-11-13 19:01 UTC (permalink / raw)
  To: Sami Tolvanen
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Kees Cook, X86 ML,
	LKML, Sedat Dilek

On 11/13/20 10:59 AM, Sami Tolvanen wrote:
> On Fri, Nov 13, 2020 at 10:35 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> On 11/13/20 10:26 AM, Sami Tolvanen wrote:
>>> e820__mapped_all is passed as a callback to is_mmconf_reserved, which
>>> expects a function of type:
>>>
>>>   typedef bool (*check_reserved_t)(u64 start, u64 end, unsigned type);
>>>
>>> This trips indirect call checking with Clang's Control-Flow Integrity
>>> (CFI). Change the last argument from enum e820_type to unsigned to fix
>>> the type mismatch.
>>
>> Hi,
>>
>> Kernel style is no raw unsigned -- use unsigned int or unsigned long, please.
>>
>> checkpatch should or could have found that issue.
> 
> It did, but the existing type definition for the callback in
> pci/mmconfig-shared.c uses raw unsigned. I can add a patch to change
> that to unsigned int as well, if you prefer.

Hm. :)
Well, yes, I prefer, but I can't require it.

thanks for the explanation.

-- 
~Randy


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/e820: fix the function type for e820__mapped_all
  2020-11-13 18:26 [PATCH] x86/e820: fix the function type for e820__mapped_all Sami Tolvanen
  2020-11-13 18:34 ` Randy Dunlap
@ 2020-11-27 12:15 ` Borislav Petkov
  2020-11-30 19:16   ` Sami Tolvanen
  1 sibling, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2020-11-27 12:15 UTC (permalink / raw)
  To: Sami Tolvanen
  Cc: Thomas Gleixner, Ingo Molnar, Kees Cook, x86, linux-kernel, Sedat Dilek

On Fri, Nov 13, 2020 at 10:26:54AM -0800, Sami Tolvanen wrote:
> e820__mapped_all is passed as a callback to is_mmconf_reserved, which
> expects a function of type:
> 
>   typedef bool (*check_reserved_t)(u64 start, u64 end, unsigned type);
> 
> This trips indirect call checking with Clang's Control-Flow Integrity
> (CFI). Change the last argument from enum e820_type to unsigned to fix
> the type mismatch.
> 
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/x86/include/asm/e820/api.h | 2 +-
>  arch/x86/kernel/e820.c          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
> index e8f58ddd06d9..e872a796619d 100644
> --- a/arch/x86/include/asm/e820/api.h
> +++ b/arch/x86/include/asm/e820/api.h
> @@ -12,7 +12,7 @@ extern unsigned long pci_mem_start;
>  
>  extern bool e820__mapped_raw_any(u64 start, u64 end, enum e820_type type);
>  extern bool e820__mapped_any(u64 start, u64 end, enum e820_type type);
> -extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
> +extern bool e820__mapped_all(u64 start, u64 end, unsigned type);

I think the proper fix is to fix the typedef to:

	typedef bool (*check_reserved_t)(u64 start, u64 end, enum e820_type type);

because

* is_mmconf_reserved() is passing in E820_TYPE_RESERVED which is enum e820_type

* is_acpi_reserved() is the other check_reserved_t function ptr and the last arg
type there is unused so it can just as well be enum e820_type.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/e820: fix the function type for e820__mapped_all
  2020-11-27 12:15 ` Borislav Petkov
@ 2020-11-30 19:16   ` Sami Tolvanen
  0 siblings, 0 replies; 6+ messages in thread
From: Sami Tolvanen @ 2020-11-30 19:16 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Thomas Gleixner, Ingo Molnar, Kees Cook, X86 ML, LKML, Sedat Dilek

On Fri, Nov 27, 2020 at 4:15 AM Borislav Petkov <bp@alien8.de> wrote:
> I think the proper fix is to fix the typedef to:
>
>         typedef bool (*check_reserved_t)(u64 start, u64 end, enum e820_type type);
>
> because
>
> * is_mmconf_reserved() is passing in E820_TYPE_RESERVED which is enum e820_type
>
> * is_acpi_reserved() is the other check_reserved_t function ptr and the last arg
> type there is unused so it can just as well be enum e820_type.

Sure, sounds good. I'll send out v3 to change the type argument to
enum e820_type.

Sami

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-30 19:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-13 18:26 [PATCH] x86/e820: fix the function type for e820__mapped_all Sami Tolvanen
2020-11-13 18:34 ` Randy Dunlap
2020-11-13 18:59   ` Sami Tolvanen
2020-11-13 19:01     ` Randy Dunlap
2020-11-27 12:15 ` Borislav Petkov
2020-11-30 19:16   ` Sami Tolvanen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).