| Message ID | 20230707131928.89500-4-deller@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: Fix fcntl64() and accept4() for 32-bit targets | expand |
On 7/7/23 14:19, Helge Deller wrote: > The mmap2() syscall allows 32-bit guests to specify the offset into a > file in page units (instead of bytes, as done by mmap(2)). > On physical machines this allows 32-bit applications to map such parts > of large files which are stored beyond the 4GB limit. > > Allow the same behaviour when emulating 32-bit guests with qemu. > > For that switch the mmap2() function to always take an abi_ullong > (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an > arithmetical overflow when shifing a 32-bit offset parameter by > 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit) > type. > > Signed-off-by: Helge Deller<deller@gmx.de> > --- > linux-user/mmap.c | 9 +++++---- > linux-user/syscall.c | 2 +- > linux-user/user-mmap.h | 2 +- > 3 files changed, 7 insertions(+), 6 deletions(-) https://patchew.org/QEMU/20230630132159.376995-1-richard.henderson@linaro.org/20230630132159.376995-12-richard.henderson@linaro.org/ Wherein I use the host off_t (which must be 64-bits). (I'm pretty sure there's an older similar patch, but I couldn't find it.) r~
On 7/7/23 21:47, Richard Henderson wrote: > On 7/7/23 14:19, Helge Deller wrote: >> The mmap2() syscall allows 32-bit guests to specify the offset into a >> file in page units (instead of bytes, as done by mmap(2)). >> On physical machines this allows 32-bit applications to map such parts >> of large files which are stored beyond the 4GB limit. >> >> Allow the same behaviour when emulating 32-bit guests with qemu. >> >> For that switch the mmap2() function to always take an abi_ullong >> (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an >> arithmetical overflow when shifing a 32-bit offset parameter by >> 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit) >> type. >> >> Signed-off-by: Helge Deller<deller@gmx.de> >> --- >> linux-user/mmap.c | 9 +++++---- >> linux-user/syscall.c | 2 +- >> linux-user/user-mmap.h | 2 +- >> 3 files changed, 7 insertions(+), 6 deletions(-) > > https://patchew.org/QEMU/20230630132159.376995-1-richard.henderson@linaro.org/20230630132159.376995-12-richard.henderson@linaro.org/ > > Wherein I use the host off_t (which must be 64-bits). I like your patch. But wouldn't it be better to use off64_t instead of off_t just to make clear that this is a 64bit int? And this part: - arg5, arg6 << MMAP_SHIFT); + arg5, (off_t)(abi_ulong)arg6 << MMAP_SHIFT); maybe should become (with brackets): ? + arg5, ((off64_t)(abi_ulong)arg6) << MMAP_SHIFT); In any case I'm fine if your or my patch could be appled. Helge
On 7/7/23 21:04, Helge Deller wrote: > On 7/7/23 21:47, Richard Henderson wrote: >> On 7/7/23 14:19, Helge Deller wrote: >>> The mmap2() syscall allows 32-bit guests to specify the offset into a >>> file in page units (instead of bytes, as done by mmap(2)). >>> On physical machines this allows 32-bit applications to map such parts >>> of large files which are stored beyond the 4GB limit. >>> >>> Allow the same behaviour when emulating 32-bit guests with qemu. >>> >>> For that switch the mmap2() function to always take an abi_ullong >>> (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an >>> arithmetical overflow when shifing a 32-bit offset parameter by >>> 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit) >>> type. >>> >>> Signed-off-by: Helge Deller<deller@gmx.de> >>> --- >>> linux-user/mmap.c | 9 +++++---- >>> linux-user/syscall.c | 2 +- >>> linux-user/user-mmap.h | 2 +- >>> 3 files changed, 7 insertions(+), 6 deletions(-) >> >> https://patchew.org/QEMU/20230630132159.376995-1-richard.henderson@linaro.org/20230630132159.376995-12-richard.henderson@linaro.org/ >> >> Wherein I use the host off_t (which must be 64-bits). > > I like your patch. > But wouldn't it be better to use off64_t instead of off_t just to make > clear that this is a 64bit int? No, I don't think so. That's the point of _FILE_OFFSET_BITS=64. > And this part: > - arg5, arg6 << MMAP_SHIFT); > + arg5, (off_t)(abi_ulong)arg6 << MMAP_SHIFT); > maybe should become (with brackets): ? > + arg5, ((off64_t)(abi_ulong)arg6) << MMAP_SHIFT); Why would you add useless parenthesis? At some point everyone should know C operator precedence... r~
diff --git a/linux-user/mmap.c b/linux-user/mmap.c index 2692936773..2750146758 100644 --- a/linux-user/mmap.c +++ b/linux-user/mmap.c @@ -192,7 +192,7 @@ error: /* map an incomplete host page */ static int mmap_frag(abi_ulong real_start, abi_ulong start, abi_ulong end, - int prot, int flags, int fd, abi_ulong offset) + int prot, int flags, int fd, abi_ullong offset) { abi_ulong real_end, addr; void *host_start; @@ -436,10 +436,11 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size, abi_ulong align) /* NOTE: all the constants are the HOST ones */ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot, - int flags, int fd, abi_ulong offset) + int flags, int fd, abi_ullong offset) { - abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len, + abi_ulong ret, end, real_start, real_end, retaddr, host_len, passthrough_start = -1, passthrough_end = -1; + abi_ullong host_offset; int page_flags, host_prot; mmap_lock(); @@ -627,7 +628,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int target_prot, /* map the middle (easier) */ if (real_start < real_end) { void *p; - unsigned long offset1; + off_t offset1; if (flags & MAP_ANONYMOUS) offset1 = 0; else diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 9e9317237d..5ebc502f71 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -10427,7 +10427,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, #endif ret = target_mmap(arg1, arg2, arg3, target_to_host_bitmask(arg4, mmap_flags_tbl), - arg5, arg6 << MMAP_SHIFT); + arg5, ((abi_ullong)arg6) << MMAP_SHIFT); return get_errno(ret); #endif case TARGET_NR_munmap: diff --git a/linux-user/user-mmap.h b/linux-user/user-mmap.h index 480ce1c114..72e99000d9 100644 --- a/linux-user/user-mmap.h +++ b/linux-user/user-mmap.h @@ -20,7 +20,7 @@ int target_mprotect(abi_ulong start, abi_ulong len, int prot); abi_long target_mmap(abi_ulong start, abi_ulong len, int prot, - int flags, int fd, abi_ulong offset); + int flags, int fd, abi_ullong offset); int target_munmap(abi_ulong start, abi_ulong len); abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size, abi_ulong new_size, unsigned long flags,
The mmap2() syscall allows 32-bit guests to specify the offset into a file in page units (instead of bytes, as done by mmap(2)). On physical machines this allows 32-bit applications to map such parts of large files which are stored beyond the 4GB limit. Allow the same behaviour when emulating 32-bit guests with qemu. For that switch the mmap2() function to always take an abi_ullong (64-bit) offset parameter for target_mmap() and mmap_frag() to avoid an arithmetical overflow when shifing a 32-bit offset parameter by 12 bits (=PAGE_SHIFT) and thus possibly overflow the abi_ulong (32-bit) type. Signed-off-by: Helge Deller <deller@gmx.de> --- linux-user/mmap.c | 9 +++++---- linux-user/syscall.c | 2 +- linux-user/user-mmap.h | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) -- 2.41.0