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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
