Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571691 > unrolled thread

[PATCH] initity: try to improve __nocapture annotations

Back to article view | Back to linux.kernel

Contents

  [PATCH] initity: try to improve __nocapture annotations Arnd Bergmann <arnd@arndb.de> - 2017-02-01 17:20 +0100
    Re: [PATCH] initity: try to improve __nocapture annotations "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-01 22:10 +0100
      Re: [PATCH] initity: try to improve __nocapture annotations Kees Cook <keescook@chromium.org> - 2017-02-01 23:50 +0100
        Re: [PATCH] initity: try to improve __nocapture annotations "Rafael J. Wysocki" <rafael@kernel.org> - 2017-02-02 00:40 +0100
          Re: [PATCH] initity: try to improve __nocapture annotations Kees Cook <keescook@chromium.org> - 2017-02-02 01:40 +0100
    Re: [PATCH] initity: try to improve __nocapture annotations Kees Cook <keescook@chromium.org> - 2017-02-02 01:50 +0100
      Re: [PATCH] initity: try to improve __nocapture annotations Arnd Bergmann <arnd@arndb.de> - 2017-02-02 13:00 +0100

#1571691 — [PATCH] initity: try to improve __nocapture annotations

There are some additional declarations that got missed in the original patch,
and some annotated functions that use the pointer is a correct but nonobvious
way:

mm/kasan/kasan.c: In function 'memmove':
mm/kasan/kasan.c:346:7: error: 'memmove' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
 void *memmove(void *dest, const void *src, size_t len)
       ^~~~~~~
mm/kasan/kasan.c: In function 'memcpy':
mm/kasan/kasan.c:355:7: error: 'memcpy' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
 void *memcpy(void *dest, const void *src, size_t len)
       ^~~~~~
drivers/acpi/acpica/utdebug.c: In function 'acpi_debug_print':
drivers/acpi/acpica/utdebug.c:158:1: error: 'acpi_debug_print' captures its 3 ('function_name') parameter, please remove it from the nocapture attribute. [-Werror]

lib/string.c:893:7: error: 'memchr_inv' captures its 1 ('start') parameter, please remove it from the nocapture attribute. [-Werror]
 void *memchr_inv(const void *start, int c, size_t bytes)
lib/string.c: In function 'strnstr':
lib/string.c:832:7: error: 'strnstr' captures its 1 ('s1') parameter, please remove it from the nocapture attribute. [-Werror]
 char *strnstr(const char *s1, const char *s2, size_t len)
       ^~~~~~~
lib/string.c:832:7: error: 'strnstr' captures its 2 ('s2') parameter, please remove it from the nocapture attribute. [-Werror]

I'm not sure if these are all appropriate fixes, please have a careful look

Fixes: c2bc07665495 ("initify: Mark functions with the __nocapture attribute")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/acpi/acpica/utdebug.c        | 2 +-
 include/acpi/acpixf.h                | 2 +-
 include/asm-generic/asm-prototypes.h | 8 ++++----
 include/linux/string.h               | 2 +-
 lib/string.c                         | 2 +-
 mm/kasan/kasan.c                     | 4 ++--
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/acpica/utdebug.c b/drivers/acpi/acpica/utdebug.c
index 044df9b0356e..de3c9cb305a2 100644
--- a/drivers/acpi/acpica/utdebug.c
+++ b/drivers/acpi/acpica/utdebug.c
@@ -154,7 +154,7 @@ static const char *acpi_ut_trim_function_name(const char *function_name)
  *
  ******************************************************************************/
 
-void ACPI_INTERNAL_VAR_XFACE
+void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE
 acpi_debug_print(u32 requested_debug_level,
 		 u32 line_number,
 		 const char *function_name,
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 9f4637e9dd92..9644cec5b082 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -946,7 +946,7 @@ ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
 						const char *module_name,
 						u32 component_id,
 						const char *format, ...))
-ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6)
+ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
 				void ACPI_INTERNAL_VAR_XFACE
 				acpi_debug_print_raw(u32 requested_debug_level,
 						     u32 line_number,
diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
index 939869c772b1..ffc0dd7e8ed2 100644
--- a/include/asm-generic/asm-prototypes.h
+++ b/include/asm-generic/asm-prototypes.h
@@ -2,12 +2,12 @@
 #undef __memset
 extern void *__memset(void *, int, __kernel_size_t);
 #undef __memcpy
-extern void *__memcpy(void *, const void *, __kernel_size_t);
+extern void *__memcpy(void *, const void *, __kernel_size_t) __nocapture(2);
 #undef __memmove
-extern void *__memmove(void *, const void *, __kernel_size_t);
+extern void *__memmove(void *, const void *, __kernel_size_t) __nocapture(2);
 #undef memset
 extern void *memset(void *, int, __kernel_size_t);
 #undef memcpy
-extern void *memcpy(void *, const void *, __kernel_size_t);
+extern void *memcpy(void *, const void *, __kernel_size_t) __nocapture(2);
 #undef memmove
-extern void *memmove(void *, const void *, __kernel_size_t);
+extern void *memmove(void *, const void *, __kernel_size_t) __nocapture(2);
diff --git a/include/linux/string.h b/include/linux/string.h
index 8b3b97e7b2b0..0ee877593464 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -76,7 +76,7 @@ static inline __must_check char *strstrip(char *str)
 extern char * strstr(const char *, const char *) __nocapture(-1, 2);
 #endif
 #ifndef __HAVE_ARCH_STRNSTR
-extern char * strnstr(const char *, const char *, size_t) __nocapture(-1, 2);
+extern char * strnstr(const char *, const char *, size_t);
 #endif
 #ifndef __HAVE_ARCH_STRLEN
 extern __kernel_size_t strlen(const char *) __nocapture(1);
diff --git a/lib/string.c b/lib/string.c
index ed83562a53ae..01151a1a0b61 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -870,7 +870,7 @@ void *memchr(const void *s, int c, size_t n)
 EXPORT_SYMBOL(memchr);
 #endif
 
-static void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)
+static __always_inline void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)
 {
 	while (bytes) {
 		if (*start != value)
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 5f6e09c88d25..ebc02ee1118e 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -343,7 +343,7 @@ void *memset(void *addr, int c, size_t len)
 }
 
 #undef memmove
-void *memmove(void *dest, const void *src, size_t len)
+__unverified_nocapture(2) void *memmove(void *dest, const void *src, size_t len)
 {
 	check_memory_region((unsigned long)src, len, false, _RET_IP_);
 	check_memory_region((unsigned long)dest, len, true, _RET_IP_);
@@ -352,7 +352,7 @@ void *memmove(void *dest, const void *src, size_t len)
 }
 
 #undef memcpy
-void *memcpy(void *dest, const void *src, size_t len)
+__unverified_nocapture(2) void *memcpy(void *dest, const void *src, size_t len)
 {
 	check_memory_region((unsigned long)src, len, false, _RET_IP_);
 	check_memory_region((unsigned long)dest, len, true, _RET_IP_);
-- 
2.9.0

[toc] | [next] | [standalone]

#1571979

On Wed, Feb 1, 2017 at 5:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> There are some additional declarations that got missed in the original patch,
> and some annotated functions that use the pointer is a correct but nonobvious
> way:
>
> mm/kasan/kasan.c: In function 'memmove':
> mm/kasan/kasan.c:346:7: error: 'memmove' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>  void *memmove(void *dest, const void *src, size_t len)
>        ^~~~~~~
> mm/kasan/kasan.c: In function 'memcpy':
> mm/kasan/kasan.c:355:7: error: 'memcpy' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>  void *memcpy(void *dest, const void *src, size_t len)
>        ^~~~~~
> drivers/acpi/acpica/utdebug.c: In function 'acpi_debug_print':
> drivers/acpi/acpica/utdebug.c:158:1: error: 'acpi_debug_print' captures its 3 ('function_name') parameter, please remove it from the nocapture attribute. [-Werror]
>
> lib/string.c:893:7: error: 'memchr_inv' captures its 1 ('start') parameter, please remove it from the nocapture attribute. [-Werror]
>  void *memchr_inv(const void *start, int c, size_t bytes)
> lib/string.c: In function 'strnstr':
> lib/string.c:832:7: error: 'strnstr' captures its 1 ('s1') parameter, please remove it from the nocapture attribute. [-Werror]
>  char *strnstr(const char *s1, const char *s2, size_t len)
>        ^~~~~~~
> lib/string.c:832:7: error: 'strnstr' captures its 2 ('s2') parameter, please remove it from the nocapture attribute. [-Werror]
>
> I'm not sure if these are all appropriate fixes, please have a careful look
>
> Fixes: c2bc07665495 ("initify: Mark functions with the __nocapture attribute")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/acpi/acpica/utdebug.c        | 2 +-
>  include/acpi/acpixf.h                | 2 +-
>  include/asm-generic/asm-prototypes.h | 8 ++++----
>  include/linux/string.h               | 2 +-
>  lib/string.c                         | 2 +-
>  mm/kasan/kasan.c                     | 4 ++--
>  6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpica/utdebug.c b/drivers/acpi/acpica/utdebug.c
> index 044df9b0356e..de3c9cb305a2 100644
> --- a/drivers/acpi/acpica/utdebug.c
> +++ b/drivers/acpi/acpica/utdebug.c
> @@ -154,7 +154,7 @@ static const char *acpi_ut_trim_function_name(const char *function_name)
>   *
>   ******************************************************************************/
>
> -void ACPI_INTERNAL_VAR_XFACE
> +void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE

Generally speaking, there is a problem with adding annotations like
this to ACPICA code.

We get that code from an external project (upstream ACPICA) and the
more Linux-specific stuff is there in it, the more difficult to
maintain it becomes.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1572029

On Wed, Feb 1, 2017 at 1:05 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Feb 1, 2017 at 5:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> There are some additional declarations that got missed in the original patch,
>> and some annotated functions that use the pointer is a correct but nonobvious
>> way:
>>
>> mm/kasan/kasan.c: In function 'memmove':
>> mm/kasan/kasan.c:346:7: error: 'memmove' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>>  void *memmove(void *dest, const void *src, size_t len)
>>        ^~~~~~~
>> mm/kasan/kasan.c: In function 'memcpy':
>> mm/kasan/kasan.c:355:7: error: 'memcpy' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>>  void *memcpy(void *dest, const void *src, size_t len)
>>        ^~~~~~
>> drivers/acpi/acpica/utdebug.c: In function 'acpi_debug_print':
>> drivers/acpi/acpica/utdebug.c:158:1: error: 'acpi_debug_print' captures its 3 ('function_name') parameter, please remove it from the nocapture attribute. [-Werror]
>>
>> lib/string.c:893:7: error: 'memchr_inv' captures its 1 ('start') parameter, please remove it from the nocapture attribute. [-Werror]
>>  void *memchr_inv(const void *start, int c, size_t bytes)
>> lib/string.c: In function 'strnstr':
>> lib/string.c:832:7: error: 'strnstr' captures its 1 ('s1') parameter, please remove it from the nocapture attribute. [-Werror]
>>  char *strnstr(const char *s1, const char *s2, size_t len)
>>        ^~~~~~~
>> lib/string.c:832:7: error: 'strnstr' captures its 2 ('s2') parameter, please remove it from the nocapture attribute. [-Werror]
>>
>> I'm not sure if these are all appropriate fixes, please have a careful look
>>
>> Fixes: c2bc07665495 ("initify: Mark functions with the __nocapture attribute")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/acpi/acpica/utdebug.c        | 2 +-
>>  include/acpi/acpixf.h                | 2 +-
>>  include/asm-generic/asm-prototypes.h | 8 ++++----
>>  include/linux/string.h               | 2 +-
>>  lib/string.c                         | 2 +-
>>  mm/kasan/kasan.c                     | 4 ++--
>>  6 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/acpi/acpica/utdebug.c b/drivers/acpi/acpica/utdebug.c
>> index 044df9b0356e..de3c9cb305a2 100644
>> --- a/drivers/acpi/acpica/utdebug.c
>> +++ b/drivers/acpi/acpica/utdebug.c
>> @@ -154,7 +154,7 @@ static const char *acpi_ut_trim_function_name(const char *function_name)
>>   *
>>   ******************************************************************************/
>>
>> -void ACPI_INTERNAL_VAR_XFACE
>> +void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE
>
> Generally speaking, there is a problem with adding annotations like
> this to ACPICA code.
>
> We get that code from an external project (upstream ACPICA) and the
> more Linux-specific stuff is there in it, the more difficult to
> maintain it becomes.

We need to find a way to solve this. Why can't take take our changes?
Or better yet, why can't we keep a delta from them if they won't take
them?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1572066

On Wed, Feb 1, 2017 at 11:44 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 1, 2017 at 1:05 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Feb 1, 2017 at 5:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> There are some additional declarations that got missed in the original patch,
>>> and some annotated functions that use the pointer is a correct but nonobvious
>>> way:
>>>
>>> mm/kasan/kasan.c: In function 'memmove':
>>> mm/kasan/kasan.c:346:7: error: 'memmove' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>>>  void *memmove(void *dest, const void *src, size_t len)
>>>        ^~~~~~~
>>> mm/kasan/kasan.c: In function 'memcpy':
>>> mm/kasan/kasan.c:355:7: error: 'memcpy' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>>>  void *memcpy(void *dest, const void *src, size_t len)
>>>        ^~~~~~
>>> drivers/acpi/acpica/utdebug.c: In function 'acpi_debug_print':
>>> drivers/acpi/acpica/utdebug.c:158:1: error: 'acpi_debug_print' captures its 3 ('function_name') parameter, please remove it from the nocapture attribute. [-Werror]
>>>
>>> lib/string.c:893:7: error: 'memchr_inv' captures its 1 ('start') parameter, please remove it from the nocapture attribute. [-Werror]
>>>  void *memchr_inv(const void *start, int c, size_t bytes)
>>> lib/string.c: In function 'strnstr':
>>> lib/string.c:832:7: error: 'strnstr' captures its 1 ('s1') parameter, please remove it from the nocapture attribute. [-Werror]
>>>  char *strnstr(const char *s1, const char *s2, size_t len)
>>>        ^~~~~~~
>>> lib/string.c:832:7: error: 'strnstr' captures its 2 ('s2') parameter, please remove it from the nocapture attribute. [-Werror]
>>>
>>> I'm not sure if these are all appropriate fixes, please have a careful look
>>>
>>> Fixes: c2bc07665495 ("initify: Mark functions with the __nocapture attribute")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>>  drivers/acpi/acpica/utdebug.c        | 2 +-
>>>  include/acpi/acpixf.h                | 2 +-
>>>  include/asm-generic/asm-prototypes.h | 8 ++++----
>>>  include/linux/string.h               | 2 +-
>>>  lib/string.c                         | 2 +-
>>>  mm/kasan/kasan.c                     | 4 ++--
>>>  6 files changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/acpi/acpica/utdebug.c b/drivers/acpi/acpica/utdebug.c
>>> index 044df9b0356e..de3c9cb305a2 100644
>>> --- a/drivers/acpi/acpica/utdebug.c
>>> +++ b/drivers/acpi/acpica/utdebug.c
>>> @@ -154,7 +154,7 @@ static const char *acpi_ut_trim_function_name(const char *function_name)
>>>   *
>>>   ******************************************************************************/
>>>
>>> -void ACPI_INTERNAL_VAR_XFACE
>>> +void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE
>>
>> Generally speaking, there is a problem with adding annotations like
>> this to ACPICA code.
>>
>> We get that code from an external project (upstream ACPICA) and the
>> more Linux-specific stuff is there in it, the more difficult to
>> maintain it becomes.
>
> We need to find a way to solve this. Why can't take take our changes?

Basically because it has to be possible to build their code using
other compilers and build environments (some of them sort of exotic).

> Or better yet, why can't we keep a delta from them if they won't take them?

The coding style of the original code is different from the kernel one
and the process used to keep track of the differences is non-trivial.
The more differences there are, the more difficult it becomes to
generate patches to backport upstream changes to the kernel code base
and the more likely it is to introduce bugs in the process which sort
of would defeat the purpose of the whole hardening exercise.

Let me reverse the question, then: Why is it necessary to annotate the
ACPICA code this way instead of just leaving it alone?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1572096

On Wed, Feb 1, 2017 at 3:38 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Feb 1, 2017 at 11:44 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Feb 1, 2017 at 1:05 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Wed, Feb 1, 2017 at 5:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> diff --git a/drivers/acpi/acpica/utdebug.c b/drivers/acpi/acpica/utdebug.c
>>>> index 044df9b0356e..de3c9cb305a2 100644
>>>> --- a/drivers/acpi/acpica/utdebug.c
>>>> +++ b/drivers/acpi/acpica/utdebug.c
>>>> @@ -154,7 +154,7 @@ static const char *acpi_ut_trim_function_name(const char *function_name)
>>>>   *
>>>>   ******************************************************************************/
>>>>
>>>> -void ACPI_INTERNAL_VAR_XFACE
>>>> +void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE
>>>
>>> Generally speaking, there is a problem with adding annotations like
>>> this to ACPICA code.
>>>
>>> We get that code from an external project (upstream ACPICA) and the
>>> more Linux-specific stuff is there in it, the more difficult to
>>> maintain it becomes.
>>
>> We need to find a way to solve this. Why can't take take our changes?
>
> Basically because it has to be possible to build their code using
> other compilers and build environments (some of them sort of exotic).

Surely those environments can support macros to make this all work sanely?

>> Or better yet, why can't we keep a delta from them if they won't take them?
>
> The coding style of the original code is different from the kernel one
> and the process used to keep track of the differences is non-trivial.
> The more differences there are, the more difficult it becomes to
> generate patches to backport upstream changes to the kernel code base
> and the more likely it is to introduce bugs in the process which sort
> of would defeat the purpose of the whole hardening exercise.
>
> Let me reverse the question, then: Why is it necessary to annotate the
> ACPICA code this way instead of just leaving it alone?

With the GCC plugins there are going to be more and more automatic
analysis of the kernel code base, and it'll require global changes to
the kernel to mark things one way or another, opt in or out of things,
etc. We need to be able to treat the kernel code as a single code
base, since that's how the plugins see it. Without this, we're
restricting the value those plugins bring.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1572101

On Wed, Feb 1, 2017 at 8:11 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> There are some additional declarations that got missed in the original patch,
> and some annotated functions that use the pointer is a correct but nonobvious
> way:
>
> mm/kasan/kasan.c: In function 'memmove':
> mm/kasan/kasan.c:346:7: error: 'memmove' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>  void *memmove(void *dest, const void *src, size_t len)
>        ^~~~~~~
> mm/kasan/kasan.c: In function 'memcpy':
> mm/kasan/kasan.c:355:7: error: 'memcpy' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
>  void *memcpy(void *dest, const void *src, size_t len)
>        ^~~~~~
> drivers/acpi/acpica/utdebug.c: In function 'acpi_debug_print':
> drivers/acpi/acpica/utdebug.c:158:1: error: 'acpi_debug_print' captures its 3 ('function_name') parameter, please remove it from the nocapture attribute. [-Werror]
>
> lib/string.c:893:7: error: 'memchr_inv' captures its 1 ('start') parameter, please remove it from the nocapture attribute. [-Werror]
>  void *memchr_inv(const void *start, int c, size_t bytes)
> lib/string.c: In function 'strnstr':
> lib/string.c:832:7: error: 'strnstr' captures its 1 ('s1') parameter, please remove it from the nocapture attribute. [-Werror]
>  char *strnstr(const char *s1, const char *s2, size_t len)
>        ^~~~~~~
> lib/string.c:832:7: error: 'strnstr' captures its 2 ('s2') parameter, please remove it from the nocapture attribute. [-Werror]
>
> I'm not sure if these are all appropriate fixes, please have a careful look
>
> Fixes: c2bc07665495 ("initify: Mark functions with the __nocapture attribute")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/acpi/acpica/utdebug.c        | 2 +-
>  include/acpi/acpixf.h                | 2 +-
>  include/asm-generic/asm-prototypes.h | 8 ++++----
>  include/linux/string.h               | 2 +-
>  lib/string.c                         | 2 +-
>  mm/kasan/kasan.c                     | 4 ++--
>  6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpica/utdebug.c b/drivers/acpi/acpica/utdebug.c
> index 044df9b0356e..de3c9cb305a2 100644
> --- a/drivers/acpi/acpica/utdebug.c
> +++ b/drivers/acpi/acpica/utdebug.c
> @@ -154,7 +154,7 @@ static const char *acpi_ut_trim_function_name(const char *function_name)
>   *
>   ******************************************************************************/
>
> -void ACPI_INTERNAL_VAR_XFACE
> +void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE
>  acpi_debug_print(u32 requested_debug_level,
>                  u32 line_number,
>                  const char *function_name,

This might be better by marking acpi_ut_trim_function_name() as
__nocapture. I'll give it a try...

> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 9f4637e9dd92..9644cec5b082 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -946,7 +946,7 @@ ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
>                                                 const char *module_name,
>                                                 u32 component_id,
>                                                 const char *format, ...))
> -ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6)
> +ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
>                                 void ACPI_INTERNAL_VAR_XFACE
>                                 acpi_debug_print_raw(u32 requested_debug_level,
>                                                      u32 line_number,

I wonder why the plugin needs this at all: function_name (the third
arg) isn't even used in the function.

> diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
> index 939869c772b1..ffc0dd7e8ed2 100644
> --- a/include/asm-generic/asm-prototypes.h
> +++ b/include/asm-generic/asm-prototypes.h
> @@ -2,12 +2,12 @@
>  #undef __memset
>  extern void *__memset(void *, int, __kernel_size_t);
>  #undef __memcpy
> -extern void *__memcpy(void *, const void *, __kernel_size_t);
> +extern void *__memcpy(void *, const void *, __kernel_size_t) __nocapture(2);
>  #undef __memmove
> -extern void *__memmove(void *, const void *, __kernel_size_t);
> +extern void *__memmove(void *, const void *, __kernel_size_t) __nocapture(2);
>  #undef memset
>  extern void *memset(void *, int, __kernel_size_t);
>  #undef memcpy
> -extern void *memcpy(void *, const void *, __kernel_size_t);
> +extern void *memcpy(void *, const void *, __kernel_size_t) __nocapture(2);
>  #undef memmove
> -extern void *memmove(void *, const void *, __kernel_size_t);
> +extern void *memmove(void *, const void *, __kernel_size_t) __nocapture(2);

Ah yeah, KASAN. This seems correct.

> diff --git a/include/linux/string.h b/include/linux/string.h
> index 8b3b97e7b2b0..0ee877593464 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -76,7 +76,7 @@ static inline __must_check char *strstrip(char *str)
>  extern char * strstr(const char *, const char *) __nocapture(-1, 2);
>  #endif
>  #ifndef __HAVE_ARCH_STRNSTR
> -extern char * strnstr(const char *, const char *, size_t) __nocapture(-1, 2);
> +extern char * strnstr(const char *, const char *, size_t);

That doesn't seem right: strnstr doesn't capture...

>  #endif
>  #ifndef __HAVE_ARCH_STRLEN
>  extern __kernel_size_t strlen(const char *) __nocapture(1);
> diff --git a/lib/string.c b/lib/string.c
> index ed83562a53ae..01151a1a0b61 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -870,7 +870,7 @@ void *memchr(const void *s, int c, size_t n)
>  EXPORT_SYMBOL(memchr);
>  #endif
>
> -static void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)
> +static __always_inline void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)

Is this from another fix? Seems unrelated?

>  {
>         while (bytes) {
>                 if (*start != value)
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 5f6e09c88d25..ebc02ee1118e 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -343,7 +343,7 @@ void *memset(void *addr, int c, size_t len)
>  }
>
>  #undef memmove
> -void *memmove(void *dest, const void *src, size_t len)
> +__unverified_nocapture(2) void *memmove(void *dest, const void *src, size_t len)
>  {
>         check_memory_region((unsigned long)src, len, false, _RET_IP_);
>         check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> @@ -352,7 +352,7 @@ void *memmove(void *dest, const void *src, size_t len)
>  }
>
>  #undef memcpy
> -void *memcpy(void *dest, const void *src, size_t len)
> +__unverified_nocapture(2) void *memcpy(void *dest, const void *src, size_t len)
>  {
>         check_memory_region((unsigned long)src, len, false, _RET_IP_);
>         check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> --
> 2.9.0
>

Thanks for the patch! I'll try to reproduce the warnings and get some
fixes built if Emese or PaX Team don't beat me to it. :)

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1572304

On Thu, Feb 2, 2017 at 1:39 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 1, 2017 at 8:11 AM, Arnd Bergmann <arnd@arndb.de> wrote:

>> -void ACPI_INTERNAL_VAR_XFACE
>> +void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE
>>  acpi_debug_print(u32 requested_debug_level,
>>                  u32 line_number,
>>                  const char *function_name,
>
> This might be better by marking acpi_ut_trim_function_name() as
> __nocapture. I'll give it a try...

I tried that without success: the problem is actually the later acpi_os_printf()
that takes the result from acpi_ut_trim_function_name().

>> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
>> index 9f4637e9dd92..9644cec5b082 100644
>> --- a/include/acpi/acpixf.h
>> +++ b/include/acpi/acpixf.h
>> @@ -946,7 +946,7 @@ ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
>>                                                 const char *module_name,
>>                                                 u32 component_id,
>>                                                 const char *format, ...))
>> -ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6)
>> +ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
>>                                 void ACPI_INTERNAL_VAR_XFACE
>>                                 acpi_debug_print_raw(u32 requested_debug_level,
>>                                                      u32 line_number,
>
> I wonder why the plugin needs this at all: function_name (the third
> arg) isn't even used in the function.

This one wasn't needed, I should probably have left it out. It just seemed
right for consistency to do the same for acpi_debug_print_raw() and
acpi_debug_print().

>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index 8b3b97e7b2b0..0ee877593464 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -76,7 +76,7 @@ static inline __must_check char *strstrip(char *str)
>>  extern char * strstr(const char *, const char *) __nocapture(-1, 2);
>>  #endif
>>  #ifndef __HAVE_ARCH_STRNSTR
>> -extern char * strnstr(const char *, const char *, size_t) __nocapture(-1, 2);
>> +extern char * strnstr(const char *, const char *, size_t);
>
> That doesn't seem right: strnstr doesn't capture...

Right, so better __unverified_nocapture() then?

>>  #endif
>>  #ifndef __HAVE_ARCH_STRLEN
>>  extern __kernel_size_t strlen(const char *) __nocapture(1);
>> diff --git a/lib/string.c b/lib/string.c
>> index ed83562a53ae..01151a1a0b61 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -870,7 +870,7 @@ void *memchr(const void *s, int c, size_t n)
>>  EXPORT_SYMBOL(memchr);
>>  #endif
>>
>> -static void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)
>> +static __always_inline void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)
>
> Is this from another fix? Seems unrelated?

This fixed a warning about memchr_inv() for me: when check_bytes8()
is inlined, the compiler can see that the argument is not captured, but
if gcc decides against inlining, it warns.

>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 5f6e09c88d25..ebc02ee1118e 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -343,7 +343,7 @@ void *memset(void *addr, int c, size_t len)
>>  }
>>
>>  #undef memmove
>> -void *memmove(void *dest, const void *src, size_t len)
>> +__unverified_nocapture(2) void *memmove(void *dest, const void *src, size_t len)
>>  {
>>         check_memory_region((unsigned long)src, len, false, _RET_IP_);
>>         check_memory_region((unsigned long)dest, len, true, _RET_IP_);
>> @@ -352,7 +352,7 @@ void *memmove(void *dest, const void *src, size_t len)
>>  }
>>
>>  #undef memcpy
>> -void *memcpy(void *dest, const void *src, size_t len)
>> +__unverified_nocapture(2) void *memcpy(void *dest, const void *src, size_t len)
>>  {
>>         check_memory_region((unsigned long)src, len, false, _RET_IP_);
>>         check_memory_region((unsigned long)dest, len, true, _RET_IP_);
>> --
>> 2.9.0
>>
>
> Thanks for the patch! I'll try to reproduce the warnings and get some
> fixes built if Emese or PaX Team don't beat me to it. :)

I later got more warnings for some of the string functions, but we can
deal with them
separately.

    Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web