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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

So I'm happy with this, but I think Cyril's comment deserves a response:

> 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 don't see why, __OPEN_NEEDS_MODE is going to be different between
functions and libc/kernel versions.

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


-- 
Thank you,
Richard.

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