Re: [PATCH] initity: try to improve __nocapture annotations

[Rafael J. Wysocki]

On Wed, Feb 1, 2017 at 11:44 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Wed, Feb 1, 2017 at 1:05 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>> On Wed, Feb 1, 2017 at 5:11 PM, Arnd Bergmann <arnd@xxxxxxxx> 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@xxxxxxxx>
>>> ---
>>>  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