| Message ID | 20230327211824.1785547-11-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg patch queue | expand |
Hi Richard, On 27/3/23 23:18, Richard Henderson wrote: > Pass the address of the last byte of the image, rather than > the first address past the last byte. This avoids overflow > when the last page of the address space is involved. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > linux-user/elfload.c | 24 ++++++++++++------------ > linux-user/flatload.c | 2 +- > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index fa4cc41567..dfae967908 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -2504,7 +2504,7 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr, > if (guest_hiaddr > reserved_va) { > error_report("%s: requires more than reserved virtual " > "address space (0x%" PRIx64 " > 0x%lx)", > - image_name, (uint64_t)guest_hiaddr, reserved_va); > + image_name, (uint64_t)guest_hiaddr + 1, reserved_va); > exit(EXIT_FAILURE); > } > } else { > @@ -2512,7 +2512,7 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr, > if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) { > error_report("%s: requires more virtual address space " > "than the host can provide (0x%" PRIx64 ")", > - image_name, (uint64_t)guest_hiaddr - guest_base); > + image_name, (uint64_t)guest_hiaddr + 1 - guest_base); > exit(EXIT_FAILURE); > } > #endif > @@ -2525,18 +2525,18 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr, > if (reserved_va) { > guest_loaddr = (guest_base >= mmap_min_addr ? 0 > : mmap_min_addr - guest_base); > - guest_hiaddr = reserved_va; > + guest_hiaddr = reserved_va - 1; > } > > /* Reserve the address space for the binary, or reserved_va. */ > test = g2h_untagged(guest_loaddr); > - addr = mmap(test, guest_hiaddr - guest_loaddr, PROT_NONE, flags, -1, 0); > + addr = mmap(test, guest_hiaddr - guest_loaddr + 1, PROT_NONE, flags, -1, 0); > if (test != addr) { > pgb_fail_in_use(image_name); > } > qemu_log_mask(CPU_LOG_PAGE, > - "%s: base @ %p for " TARGET_ABI_FMT_ld " bytes\n", > - __func__, addr, guest_hiaddr - guest_loaddr); > + "%s: base @ %p for %" PRIu64 " bytes\n", > + __func__, addr, (uint64_t)guest_hiaddr - guest_loaddr + 1); > } > > /** > @@ -2680,7 +2680,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, > if (hiaddr != orig_hiaddr) { > error_report("%s: requires virtual address space that the " > "host cannot provide (0x%" PRIx64 ")", > - image_name, (uint64_t)orig_hiaddr); > + image_name, (uint64_t)orig_hiaddr + 1); > exit(EXIT_FAILURE); > } > > @@ -2694,7 +2694,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, > * arithmetic wraps around. > */ > if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) { > - hiaddr = (uintptr_t) 4 << 30; > + hiaddr = UINT32_MAX; > } else { > offset = -(HI_COMMPAGE & -align); > } > @@ -2702,7 +2702,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, > loaddr = MIN(loaddr, LO_COMMPAGE & -align); > } > > - addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset); > + addr = pgb_find_hole(loaddr, hiaddr - loaddr + 1, align, offset); > if (addr == -1) { > /* > * If HI_COMMPAGE, there *might* be a non-consecutive allocation > @@ -2755,7 +2755,7 @@ static void pgb_reserved_va(const char *image_name, abi_ulong guest_loaddr, > if (guest_hiaddr > reserved_va) { > error_report("%s: requires more than reserved virtual " > "address space (0x%" PRIx64 " > 0x%lx)", > - image_name, (uint64_t)guest_hiaddr, reserved_va); > + image_name, (uint64_t)guest_hiaddr + 1, reserved_va); > exit(EXIT_FAILURE); > } > > @@ -3021,7 +3021,7 @@ static void load_elf_image(const char *image_name, int image_fd, > if (a < loaddr) { > loaddr = a; > } > - a = eppnt->p_vaddr + eppnt->p_memsz; > + a = eppnt->p_vaddr + eppnt->p_memsz - 1; > if (a > hiaddr) { > hiaddr = a; > } > @@ -3112,7 +3112,7 @@ static void load_elf_image(const char *image_name, int image_fd, > * In both cases, we will overwrite pages in this range with mappings > * from the executable. > */ > - load_addr = target_mmap(loaddr, hiaddr - loaddr, PROT_NONE, > + load_addr = target_mmap(loaddr, (size_t)hiaddr - loaddr + 1, PROT_NONE, > MAP_PRIVATE | MAP_ANON | MAP_NORESERVE | > (ehdr->e_type == ET_EXEC ? MAP_FIXED : 0), > -1, 0); > diff --git a/linux-user/flatload.c b/linux-user/flatload.c > index e99570ca18..5efec2630e 100644 > --- a/linux-user/flatload.c > +++ b/linux-user/flatload.c > @@ -448,7 +448,7 @@ static int load_flat_file(struct linux_binprm * bprm, > * Allocate the address space. > */ > probe_guest_base(bprm->filename, 0, > - text_len + data_len + extra + indx_len); > + text_len + data_len + extra + indx_len - 1); Per linux-user/user-internals.h: /** * probe_guest_base: * @image_name: the executable being loaded * @loaddr: the lowest fixed address in the executable * @hiaddr: the highest fixed address in the executable * * Creates the initial guest address space in the host memory space. * * If @loaddr == 0, then no address in the executable is fixed, * i.e. it is fully relocatable. In that case @hiaddr is the size * of the executable. * * This function will not return if a valid value for guest_base * cannot be chosen. On return, the executable loader can expect * * target_mmap(loaddr, hiaddr - loaddr, ...) * * to succeed. */ Since here @loaddr == 0, "@hiaddr is the size of the executable", not "the first address past the last byte". So we can not "Pass the address of the last byte of the image" to this API. Maybe the API description is incorrect, in that case your patch is right. Otherwise we might need to tune probe_guest_base(). > > /* > * there are a couple of cases here, the separate code/data
On 3/28/23 06:51, Philippe Mathieu-Daudé wrote: > Hi Richard, > > On 27/3/23 23:18, Richard Henderson wrote: >> Pass the address of the last byte of the image, rather than >> the first address past the last byte. This avoids overflow >> when the last page of the address space is involved. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> linux-user/elfload.c | 24 ++++++++++++------------ >> linux-user/flatload.c | 2 +- >> 2 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/linux-user/elfload.c b/linux-user/elfload.c >> index fa4cc41567..dfae967908 100644 >> --- a/linux-user/elfload.c >> +++ b/linux-user/elfload.c >> @@ -2504,7 +2504,7 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong >> guest_loaddr, >> if (guest_hiaddr > reserved_va) { >> error_report("%s: requires more than reserved virtual " >> "address space (0x%" PRIx64 " > 0x%lx)", >> - image_name, (uint64_t)guest_hiaddr, reserved_va); >> + image_name, (uint64_t)guest_hiaddr + 1, reserved_va); >> exit(EXIT_FAILURE); >> } >> } else { >> @@ -2512,7 +2512,7 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong >> guest_loaddr, >> if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) { >> error_report("%s: requires more virtual address space " >> "than the host can provide (0x%" PRIx64 ")", >> - image_name, (uint64_t)guest_hiaddr - guest_base); >> + image_name, (uint64_t)guest_hiaddr + 1 - guest_base); >> exit(EXIT_FAILURE); >> } >> #endif >> @@ -2525,18 +2525,18 @@ static void pgb_have_guest_base(const char *image_name, >> abi_ulong guest_loaddr, >> if (reserved_va) { >> guest_loaddr = (guest_base >= mmap_min_addr ? 0 >> : mmap_min_addr - guest_base); >> - guest_hiaddr = reserved_va; >> + guest_hiaddr = reserved_va - 1; >> } >> /* Reserve the address space for the binary, or reserved_va. */ >> test = g2h_untagged(guest_loaddr); >> - addr = mmap(test, guest_hiaddr - guest_loaddr, PROT_NONE, flags, -1, 0); >> + addr = mmap(test, guest_hiaddr - guest_loaddr + 1, PROT_NONE, flags, -1, 0); >> if (test != addr) { >> pgb_fail_in_use(image_name); >> } >> qemu_log_mask(CPU_LOG_PAGE, >> - "%s: base @ %p for " TARGET_ABI_FMT_ld " bytes\n", >> - __func__, addr, guest_hiaddr - guest_loaddr); >> + "%s: base @ %p for %" PRIu64 " bytes\n", >> + __func__, addr, (uint64_t)guest_hiaddr - guest_loaddr + 1); >> } >> /** >> @@ -2680,7 +2680,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, >> if (hiaddr != orig_hiaddr) { >> error_report("%s: requires virtual address space that the " >> "host cannot provide (0x%" PRIx64 ")", >> - image_name, (uint64_t)orig_hiaddr); >> + image_name, (uint64_t)orig_hiaddr + 1); >> exit(EXIT_FAILURE); >> } >> @@ -2694,7 +2694,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, >> * arithmetic wraps around. >> */ >> if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) { >> - hiaddr = (uintptr_t) 4 << 30; >> + hiaddr = UINT32_MAX; >> } else { >> offset = -(HI_COMMPAGE & -align); >> } >> @@ -2702,7 +2702,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, >> loaddr = MIN(loaddr, LO_COMMPAGE & -align); >> } >> - addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset); >> + addr = pgb_find_hole(loaddr, hiaddr - loaddr + 1, align, offset); >> if (addr == -1) { >> /* >> * If HI_COMMPAGE, there *might* be a non-consecutive allocation >> @@ -2755,7 +2755,7 @@ static void pgb_reserved_va(const char *image_name, abi_ulong >> guest_loaddr, >> if (guest_hiaddr > reserved_va) { >> error_report("%s: requires more than reserved virtual " >> "address space (0x%" PRIx64 " > 0x%lx)", >> - image_name, (uint64_t)guest_hiaddr, reserved_va); >> + image_name, (uint64_t)guest_hiaddr + 1, reserved_va); >> exit(EXIT_FAILURE); >> } >> @@ -3021,7 +3021,7 @@ static void load_elf_image(const char *image_name, int image_fd, >> if (a < loaddr) { >> loaddr = a; >> } >> - a = eppnt->p_vaddr + eppnt->p_memsz; >> + a = eppnt->p_vaddr + eppnt->p_memsz - 1; >> if (a > hiaddr) { >> hiaddr = a; >> } >> @@ -3112,7 +3112,7 @@ static void load_elf_image(const char *image_name, int image_fd, >> * In both cases, we will overwrite pages in this range with mappings >> * from the executable. >> */ >> - load_addr = target_mmap(loaddr, hiaddr - loaddr, PROT_NONE, >> + load_addr = target_mmap(loaddr, (size_t)hiaddr - loaddr + 1, PROT_NONE, >> MAP_PRIVATE | MAP_ANON | MAP_NORESERVE | >> (ehdr->e_type == ET_EXEC ? MAP_FIXED : 0), >> -1, 0); >> diff --git a/linux-user/flatload.c b/linux-user/flatload.c >> index e99570ca18..5efec2630e 100644 >> --- a/linux-user/flatload.c >> +++ b/linux-user/flatload.c >> @@ -448,7 +448,7 @@ static int load_flat_file(struct linux_binprm * bprm, >> * Allocate the address space. >> */ >> probe_guest_base(bprm->filename, 0, >> - text_len + data_len + extra + indx_len); >> + text_len + data_len + extra + indx_len - 1); > > Per linux-user/user-internals.h: > > /** > * probe_guest_base: > * @image_name: the executable being loaded > * @loaddr: the lowest fixed address in the executable > * @hiaddr: the highest fixed address in the executable > * > * Creates the initial guest address space in the host memory space. > * > * If @loaddr == 0, then no address in the executable is fixed, > * i.e. it is fully relocatable. In that case @hiaddr is the size > * of the executable. > * > * This function will not return if a valid value for guest_base > * cannot be chosen. On return, the executable loader can expect > * > * target_mmap(loaddr, hiaddr - loaddr, ...) > * > * to succeed. > */ > > Since here @loaddr == 0, "@hiaddr is the size of the executable", > not "the first address past the last byte". > > So we can not "Pass the address of the last byte of the image" > to this API. Maybe the API description is incorrect, in that > case your patch is right. Otherwise we might need to tune > probe_guest_base(). I'll update the documentation. r~
On 28/3/23 19:47, Richard Henderson wrote: > On 3/28/23 06:51, Philippe Mathieu-Daudé wrote: >> Hi Richard, >> >> On 27/3/23 23:18, Richard Henderson wrote: >>> Pass the address of the last byte of the image, rather than >>> the first address past the last byte. This avoids overflow >>> when the last page of the address space is involved. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> linux-user/elfload.c | 24 ++++++++++++------------ >>> linux-user/flatload.c | 2 +- >>> 2 files changed, 13 insertions(+), 13 deletions(-) >>> index e99570ca18..5efec2630e 100644 >>> --- a/linux-user/flatload.c >>> +++ b/linux-user/flatload.c >>> @@ -448,7 +448,7 @@ static int load_flat_file(struct linux_binprm * >>> bprm, >>> * Allocate the address space. >>> */ >>> probe_guest_base(bprm->filename, 0, >>> - text_len + data_len + extra + indx_len); >>> + text_len + data_len + extra + indx_len - 1); >> >> Per linux-user/user-internals.h: >> >> /** >> * probe_guest_base: >> * @image_name: the executable being loaded >> * @loaddr: the lowest fixed address in the executable >> * @hiaddr: the highest fixed address in the executable >> * >> * Creates the initial guest address space in the host memory space. >> * >> * If @loaddr == 0, then no address in the executable is fixed, >> * i.e. it is fully relocatable. In that case @hiaddr is the size >> * of the executable. >> * >> * This function will not return if a valid value for guest_base >> * cannot be chosen. On return, the executable loader can expect >> * >> * target_mmap(loaddr, hiaddr - loaddr, ...) >> * >> * to succeed. >> */ >> >> Since here @loaddr == 0, "@hiaddr is the size of the executable", >> not "the first address past the last byte". >> >> So we can not "Pass the address of the last byte of the image" >> to this API. Maybe the API description is incorrect, in that >> case your patch is right. Otherwise we might need to tune >> probe_guest_base(). > > I'll update the documentation. Then: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index fa4cc41567..dfae967908 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -2504,7 +2504,7 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr, if (guest_hiaddr > reserved_va) { error_report("%s: requires more than reserved virtual " "address space (0x%" PRIx64 " > 0x%lx)", - image_name, (uint64_t)guest_hiaddr, reserved_va); + image_name, (uint64_t)guest_hiaddr + 1, reserved_va); exit(EXIT_FAILURE); } } else { @@ -2512,7 +2512,7 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr, if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) { error_report("%s: requires more virtual address space " "than the host can provide (0x%" PRIx64 ")", - image_name, (uint64_t)guest_hiaddr - guest_base); + image_name, (uint64_t)guest_hiaddr + 1 - guest_base); exit(EXIT_FAILURE); } #endif @@ -2525,18 +2525,18 @@ static void pgb_have_guest_base(const char *image_name, abi_ulong guest_loaddr, if (reserved_va) { guest_loaddr = (guest_base >= mmap_min_addr ? 0 : mmap_min_addr - guest_base); - guest_hiaddr = reserved_va; + guest_hiaddr = reserved_va - 1; } /* Reserve the address space for the binary, or reserved_va. */ test = g2h_untagged(guest_loaddr); - addr = mmap(test, guest_hiaddr - guest_loaddr, PROT_NONE, flags, -1, 0); + addr = mmap(test, guest_hiaddr - guest_loaddr + 1, PROT_NONE, flags, -1, 0); if (test != addr) { pgb_fail_in_use(image_name); } qemu_log_mask(CPU_LOG_PAGE, - "%s: base @ %p for " TARGET_ABI_FMT_ld " bytes\n", - __func__, addr, guest_hiaddr - guest_loaddr); + "%s: base @ %p for %" PRIu64 " bytes\n", + __func__, addr, (uint64_t)guest_hiaddr - guest_loaddr + 1); } /** @@ -2680,7 +2680,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, if (hiaddr != orig_hiaddr) { error_report("%s: requires virtual address space that the " "host cannot provide (0x%" PRIx64 ")", - image_name, (uint64_t)orig_hiaddr); + image_name, (uint64_t)orig_hiaddr + 1); exit(EXIT_FAILURE); } @@ -2694,7 +2694,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, * arithmetic wraps around. */ if (sizeof(uintptr_t) == 8 || loaddr >= 0x80000000u) { - hiaddr = (uintptr_t) 4 << 30; + hiaddr = UINT32_MAX; } else { offset = -(HI_COMMPAGE & -align); } @@ -2702,7 +2702,7 @@ static void pgb_static(const char *image_name, abi_ulong orig_loaddr, loaddr = MIN(loaddr, LO_COMMPAGE & -align); } - addr = pgb_find_hole(loaddr, hiaddr - loaddr, align, offset); + addr = pgb_find_hole(loaddr, hiaddr - loaddr + 1, align, offset); if (addr == -1) { /* * If HI_COMMPAGE, there *might* be a non-consecutive allocation @@ -2755,7 +2755,7 @@ static void pgb_reserved_va(const char *image_name, abi_ulong guest_loaddr, if (guest_hiaddr > reserved_va) { error_report("%s: requires more than reserved virtual " "address space (0x%" PRIx64 " > 0x%lx)", - image_name, (uint64_t)guest_hiaddr, reserved_va); + image_name, (uint64_t)guest_hiaddr + 1, reserved_va); exit(EXIT_FAILURE); } @@ -3021,7 +3021,7 @@ static void load_elf_image(const char *image_name, int image_fd, if (a < loaddr) { loaddr = a; } - a = eppnt->p_vaddr + eppnt->p_memsz; + a = eppnt->p_vaddr + eppnt->p_memsz - 1; if (a > hiaddr) { hiaddr = a; } @@ -3112,7 +3112,7 @@ static void load_elf_image(const char *image_name, int image_fd, * In both cases, we will overwrite pages in this range with mappings * from the executable. */ - load_addr = target_mmap(loaddr, hiaddr - loaddr, PROT_NONE, + load_addr = target_mmap(loaddr, (size_t)hiaddr - loaddr + 1, PROT_NONE, MAP_PRIVATE | MAP_ANON | MAP_NORESERVE | (ehdr->e_type == ET_EXEC ? MAP_FIXED : 0), -1, 0); diff --git a/linux-user/flatload.c b/linux-user/flatload.c index e99570ca18..5efec2630e 100644 --- a/linux-user/flatload.c +++ b/linux-user/flatload.c @@ -448,7 +448,7 @@ static int load_flat_file(struct linux_binprm * bprm, * Allocate the address space. */ probe_guest_base(bprm->filename, 0, - text_len + data_len + extra + indx_len); + text_len + data_len + extra + indx_len - 1); /* * there are a couple of cases here, the separate code/data
Pass the address of the last byte of the image, rather than the first address past the last byte. This avoids overflow when the last page of the address space is involved. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/elfload.c | 24 ++++++++++++------------ linux-user/flatload.c | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-)