| Message ID | 20221129130350.219082-1-tudor.cretu@arm.com |
|---|---|
| Headers | show |
| Series | safe_macros: Fix undefined behaviour in vararg handling | expand |
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
Hi all, > Hello, > So I'm happy with this, but I think Cyril's comment deserves a response: +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. +1 > I don't see why, __OPEN_NEEDS_MODE is going to be different between > functions and libc/kernel versions. 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). 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
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. 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
> 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! 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
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
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(-)