Re: [LTP] [PATCH v2 0/3] safe_macros: Fix undefined behaviour in vararg handling

[Tudor Cretu <tudor.cretu@arm.com>]

On 29-11-2022 15:15, Petr Vorel wrote:
> 
> 
>> On 29-11-2022 13:59, Petr Vorel wrote:
>>> Hi all,
> 
>>>> Hello,
> 
>>>> So I'm happy with this, but I think Cyril's comment deserves a response:
> 
>> Indeed, I noticed it too late after sending the v2.
> 
>>> +1
> 
>>>>> Looking at how glibc handles this, the code looks like:
> 
>>>>> 	int mode = 0;
> 
>>>>> 	if (__OPEN_NEEDS_MODE(oflag)) {
>>>>> 		..
>>>>> 		mode = va_arg(arg, int);
>>>>> 		..
>>>>> 	}
> 
>>>>> That sounds much easier than messing with the macros and should avoid
>>>>> undefined behavior.
> 
>> I considered this and I think it's better to focus strictly on the handling
>> the variadicness issue, and wanted to avoid duplicating logic from libcs.
> 
>>> +1
> 
>>>> I don't see why, __OPEN_NEEDS_MODE is going to be different between
> 
>>>> functions and libc/kernel versions.
> 
>> Haven't thought about that, that's a good point in my opinion.
> 
> 
>>> Looking at glibc's __OPEN_NEEDS_MODE definition, the logic is obviously the same
>>> as musl code for open (it just use O_TMPFILE instead of __O_TMPFILE therefore no
>>> need to check for #ifdef __O_TMPFILE).
> 
>> I agree, for open/openat this approach would be fairly simple, there is
>> semctl too though, I'll need to have a look how glibc and musl handle it.
> 
> Thanks a lot for your time Tudor!

My pleasure! Many thanks for the feedback, all. I have posted a v3. I 
look forward to your opinions.

Kind regards,
Tudor

> 
> Kind regards,
> Petr
> 
>> Kind regards,
>> Tudor
> 
> 
>>> Kind regards,
>>> Petr
> 
>>>> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
> 
>>>> Tudor Cretu <tudor.cretu@arm.com> writes:
> 
>>>>> Accessing elements in an empty va_list results in undefined behaviour[0]
>>>>> that can include accessing arbitrary stack memory. While typically this
>>>>> doesn't raise a fault, some new more security-oriented architectures
>>>>> (e.g. CHERI[1] or Morello[2]) don't allow it.
> 
>>>>> Therefore, remove the variadicness from safe_* wrappers that always call
>>>>> the functions with the optional argument included.
> 
>>>>> Adapt the respective SAFE_* macros to handle the change by passing a
>>>>> default argument if they're omitted.
> 
>>>>> [0]: [ISO/IEC 9899:2011] Programming Languages—C, 3rd ed, paragraph 7.16.1.1
>>>>> [1]: https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/
>>>>> [2]: https://www.morello-project.org/
> 
>>>>> v2..v1:
>>>>>     - PATCH 1: Remove the NULL argument for mode from SAFE_OPEN instances
>>>>>       to avoid the pointer to int conversion.
> 
>>>>> Tudor Cretu (3):
>>>>>     safe_open: Fix undefined behaviour in vararg handling
>>>>>     safe_openat: Fix undefined behaviour in vararg handling
>>>>>     safe_semctl: Fix undefined behaviour in vararg handling
> 
>>>>>    include/old/safe_macros.h                         |  6 ++++--
>>>>>    include/safe_macros_fn.h                          |  3 ++-
>>>>>    include/tst_safe_file_at.h                        | 10 ++++++----
>>>>>    include/tst_safe_macros.h                         |  6 ++++--
>>>>>    include/tst_safe_sysv_ipc.h                       | 14 +++++++++-----
>>>>>    lib/safe_macros.c                                 | 13 +------------
>>>>>    lib/tst_cgroup.c                                  |  2 +-
>>>>>    lib/tst_safe_file_at.c                            | 11 +++--------
>>>>>    lib/tst_safe_sysv_ipc.c                           | 10 +---------
>>>>>    testcases/kernel/syscalls/fgetxattr/fgetxattr01.c |  2 +-
>>>>>    testcases/kernel/syscalls/fgetxattr/fgetxattr02.c |  2 +-
>>>>>    testcases/kernel/syscalls/fgetxattr/fgetxattr03.c |  2 +-
>>>>>    testcases/kernel/syscalls/fsetxattr/fsetxattr01.c |  2 +-
>>>>>    testcases/kernel/syscalls/fsetxattr/fsetxattr02.c |  2 +-
>>>>>    14 files changed, 36 insertions(+), 49 deletions(-)
> 
>>>>> -- 
>>>>> 2.25.1

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp