| Message ID | 20200424205755.GA26282@ls3530.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: Drop unnecessary check in dup3 syscall | expand |
On 4/24/20 3:57 PM, Helge Deller wrote: > Drop the extra check in dup3() if anything other than FD_CLOEXEC (aka > O_CLOEXEC) was given. Instead simply rely on any error codes returned by > the host dup3() syscall. > > Signed-off-by: Helge Deller <deller@gmx.de> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 05f03919ff..ebf0d38321 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -8301,12 +8310,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > #if defined(CONFIG_DUP3) && defined(TARGET_NR_dup3) > case TARGET_NR_dup3: > { > - int host_flags; > - > - if ((arg3 & ~TARGET_O_CLOEXEC) != 0) { > - return -EINVAL; > - } > - host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); > + int host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); I don't think this is quite correct. target_to_host_bitmask() silently ignores unknown bits, and a user that was relying on bit 0x40000000 to cause an EINVAL will not fail with this change (unless bit 0x40000000 happens to be one of the bits translated by fcntl_flags_tbl). The open() syscall is notorious for ignoring unknown bits rather than failing with EINVAL, and it is has come back to haunt kernel developers; newer syscalls like dup3() learned from the mistake, and we really do want to catch unsupported bits up to make it easier for future kernels to define meanings to those bits without them being silently swallowed when run on older systems that did not know what those bits meant.
On 24.04.20 23:32, Eric Blake wrote: > On 4/24/20 3:57 PM, Helge Deller wrote: >> Drop the extra check in dup3() if anything other than FD_CLOEXEC (aka >> O_CLOEXEC) was given. Instead simply rely on any error codes returned by >> the host dup3() syscall. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 05f03919ff..ebf0d38321 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -8301,12 +8310,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, >> #if defined(CONFIG_DUP3) && defined(TARGET_NR_dup3) >> case TARGET_NR_dup3: >> { >> - int host_flags; >> - >> - if ((arg3 & ~TARGET_O_CLOEXEC) != 0) { >> - return -EINVAL; >> - } >> - host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); >> + int host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); > > I don't think this is quite correct. target_to_host_bitmask() > silently ignores unknown bits, and a user that was relying on bit > 0x40000000 to cause an EINVAL will not fail with this change (unless > bit 0x40000000 happens to be one of the bits translated by > fcntl_flags_tbl). True. > The open() syscall is notorious for ignoring unknown bits rather than > failing with EINVAL, and it is has come back to haunt kernel > developers; newer syscalls like dup3() learned from the mistake, and > we really do want to catch unsupported bits up to make it easier for > future kernels to define meanings to those bits without them being > silently swallowed when run on older systems that did not know what > those bits meant. Ok, I wasn't aware that it's a design goal to manually find such cases of wrong userspace applications. But in this case, you're right that my patch shouldn't be applied. While looking at the code I just noticed another bug too, which needs fixing then: >> - if ((arg3 & ~TARGET_O_CLOEXEC) != 0) { >> - return -EINVAL; this needs to be: >> - return -TARGET_EINVAL; Helge
On 4/24/20 4:47 PM, Helge Deller wrote: >>> - host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); >>> + int host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); >> >> I don't think this is quite correct. target_to_host_bitmask() >> silently ignores unknown bits, and a user that was relying on bit >> 0x40000000 to cause an EINVAL will not fail with this change (unless >> bit 0x40000000 happens to be one of the bits translated by >> fcntl_flags_tbl). > > True. > >> The open() syscall is notorious for ignoring unknown bits rather than >> failing with EINVAL, and it is has come back to haunt kernel >> developers; newer syscalls like dup3() learned from the mistake, and >> we really do want to catch unsupported bits up to make it easier for >> future kernels to define meanings to those bits without them being >> silently swallowed when run on older systems that did not know what >> those bits meant. > Ok, I wasn't aware that it's a design goal to manually find such > cases of wrong userspace applications. But in this case, you're right > that my patch shouldn't be applied. This, and several similar ones that you also posted. Maybe you could add a new int target_to_host_bitmask_strict(int src, translate_tbl, int *dst), which returns 0 when *dst is bit-for-bit translated from src, and returns -1 if src had bits not specified by translate_tbl. In that case, the caller can then translate all usual bits and rely on the syscall() failure (as you tried here), but you can also flag -TARGET_EINVAL up front for bits not covered by the table. > > While looking at the code I just noticed another bug too, which needs > fixing then: >>> - if ((arg3 & ~TARGET_O_CLOEXEC) != 0) { >>> - return -EINVAL; > this needs to be: >>> - return -TARGET_EINVAL; Indeed. Good catch.
On Fri, 24 Apr 2020 at 22:33, Eric Blake <eblake@redhat.com> wrote: > I don't think this is quite correct. target_to_host_bitmask() silently > ignores unknown bits, and a user that was relying on bit 0x40000000 to > cause an EINVAL will not fail with this change (unless bit 0x40000000 > happens to be one of the bits translated by fcntl_flags_tbl). The > open() syscall is notorious for ignoring unknown bits rather than > failing with EINVAL, and it is has come back to haunt kernel developers; > newer syscalls like dup3() learned from the mistake, and we really do > want to catch unsupported bits up to make it easier for future kernels > to define meanings to those bits without them being silently swallowed > when run on older systems that did not know what those bits meant. The other reason linux-user sometimes has this sort of manual check of input values is that it can affect which errno value is returned if a call has multiple wrong things (eg a bad address to a pointer parameter and a bad flags value), and some test suites care about the difference. I'm not sure that's the case here, though. I didn't write out my reasoning back in 2017 when I made this page and don't remember it now, but my guess is that it's just that dup3 is only supposed to permit O_CLOEXEC, not any of the other flags that the fcntl_flags_tbl permits and translates. thanks -- PMM
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 05f03919ff..ebf0d38321 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -8301,12 +8310,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, #if defined(CONFIG_DUP3) && defined(TARGET_NR_dup3) case TARGET_NR_dup3: { - int host_flags; - - if ((arg3 & ~TARGET_O_CLOEXEC) != 0) { - return -EINVAL; - } - host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); + int host_flags = target_to_host_bitmask(arg3, fcntl_flags_tbl); ret = get_errno(dup3(arg1, arg2, host_flags)); if (ret >= 0) { fd_trans_dup(arg1, arg2);
Drop the extra check in dup3() if anything other than FD_CLOEXEC (aka O_CLOEXEC) was given. Instead simply rely on any error codes returned by the host dup3() syscall. Signed-off-by: Helge Deller <deller@gmx.de>