| Message ID | 20230807163705.9848-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user: image mapping fixes | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > Use this as extra protection for the guest mapping over > any qemu host mappings. > > Tested-by: Helge Deller <deller@gmx.de> > Reviewed-by: Helge Deller <deller@gmx.de> > Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > linux-user/elfload.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index 36e4026f05..1b4bb2d5af 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -3147,8 +3147,11 @@ static void load_elf_image(const char *image_name, int image_fd, > /* > * Reserve address space for all of this. > * > - * In the case of ET_EXEC, we supply MAP_FIXED so that we get > - * exactly the address range that is required. > + * In the case of ET_EXEC, we supply MAP_FIXED_NOREPLACE so that we get > + * exactly the address range that is required. Without reserved_va, > + * the guest address space is not isolated. We have attempted to avoid > + * conflict with the host program itself via probe_guest_base, but using > + * MAP_FIXED_NOREPLACE instead of MAP_FIXED provides an extra check. > * > * Otherwise this is ET_DYN, and we are searching for a location > * that can hold the memory space required. If the image is > @@ -3160,7 +3163,7 @@ static void load_elf_image(const char *image_name, int image_fd, > */ > 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), > + (ehdr->e_type == ET_EXEC ? MAP_FIXED_NOREPLACE : 0), > -1, 0); We should probably also check the result == load_addr for the places where MAP_FIXED_NOREPLACE isn't supported as we have this in osdep.h: #ifndef MAP_FIXED_NOREPLACE #define MAP_FIXED_NOREPLACE 0 #endif See 2667e069e7 (linux-user: don't use MAP_FIXED in pgd_find_hole_fallback) > if (load_addr == -1) { > goto exit_mmap;
On 2023/08/08 18:43, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> Use this as extra protection for the guest mapping over >> any qemu host mappings. >> >> Tested-by: Helge Deller <deller@gmx.de> >> Reviewed-by: Helge Deller <deller@gmx.de> >> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> linux-user/elfload.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/linux-user/elfload.c b/linux-user/elfload.c >> index 36e4026f05..1b4bb2d5af 100644 >> --- a/linux-user/elfload.c >> +++ b/linux-user/elfload.c >> @@ -3147,8 +3147,11 @@ static void load_elf_image(const char *image_name, int image_fd, >> /* >> * Reserve address space for all of this. >> * >> - * In the case of ET_EXEC, we supply MAP_FIXED so that we get >> - * exactly the address range that is required. >> + * In the case of ET_EXEC, we supply MAP_FIXED_NOREPLACE so that we get >> + * exactly the address range that is required. Without reserved_va, >> + * the guest address space is not isolated. We have attempted to avoid >> + * conflict with the host program itself via probe_guest_base, but using >> + * MAP_FIXED_NOREPLACE instead of MAP_FIXED provides an extra check. >> * >> * Otherwise this is ET_DYN, and we are searching for a location >> * that can hold the memory space required. If the image is >> @@ -3160,7 +3163,7 @@ static void load_elf_image(const char *image_name, int image_fd, >> */ >> 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), >> + (ehdr->e_type == ET_EXEC ? MAP_FIXED_NOREPLACE : 0), >> -1, 0); > > We should probably also check the result == load_addr for the places > where MAP_FIXED_NOREPLACE isn't supported as we have this in osdep.h: > > #ifndef MAP_FIXED_NOREPLACE > #define MAP_FIXED_NOREPLACE 0 > #endif > > See 2667e069e7 (linux-user: don't use MAP_FIXED in pgd_find_hole_fallback) It assumes target_mmap() emulates MAP_FIXED_NOREPLACE when the host does not support it as commit e69e032d1a ("linux-user: Use MAP_FIXED_NOREPLACE for do_brk()") already does, but defining MAP_FIXED_NOREPLACE zero breaks such emulation. I wrote a fix: https://patchew.org/QEMU/20230808115242.73025-1-akihiko.odaki@daynix.com/ > >> if (load_addr == -1) { >> goto exit_mmap; > >
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2023/08/08 18:43, Alex Bennée wrote: >> Richard Henderson <richard.henderson@linaro.org> writes: >> >>> Use this as extra protection for the guest mapping over >>> any qemu host mappings. >>> >>> Tested-by: Helge Deller <deller@gmx.de> >>> Reviewed-by: Helge Deller <deller@gmx.de> >>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> linux-user/elfload.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c >>> index 36e4026f05..1b4bb2d5af 100644 >>> --- a/linux-user/elfload.c >>> +++ b/linux-user/elfload.c >>> @@ -3147,8 +3147,11 @@ static void load_elf_image(const char *image_name, int image_fd, >>> /* >>> * Reserve address space for all of this. >>> * >>> - * In the case of ET_EXEC, we supply MAP_FIXED so that we get >>> - * exactly the address range that is required. >>> + * In the case of ET_EXEC, we supply MAP_FIXED_NOREPLACE so that we get >>> + * exactly the address range that is required. Without reserved_va, >>> + * the guest address space is not isolated. We have attempted to avoid >>> + * conflict with the host program itself via probe_guest_base, but using >>> + * MAP_FIXED_NOREPLACE instead of MAP_FIXED provides an extra check. >>> * >>> * Otherwise this is ET_DYN, and we are searching for a location >>> * that can hold the memory space required. If the image is >>> @@ -3160,7 +3163,7 @@ static void load_elf_image(const char *image_name, int image_fd, >>> */ >>> 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), >>> + (ehdr->e_type == ET_EXEC ? MAP_FIXED_NOREPLACE : 0), >>> -1, 0); >> We should probably also check the result == load_addr for the places >> where MAP_FIXED_NOREPLACE isn't supported as we have this in osdep.h: >> #ifndef MAP_FIXED_NOREPLACE >> #define MAP_FIXED_NOREPLACE 0 >> #endif >> See 2667e069e7 (linux-user: don't use MAP_FIXED in >> pgd_find_hole_fallback) > > It assumes target_mmap() emulates MAP_FIXED_NOREPLACE when the host > does not support it as commit e69e032d1a ("linux-user: Use > MAP_FIXED_NOREPLACE for do_brk()") already does, but defining > MAP_FIXED_NOREPLACE zero breaks such emulation. I wrote a fix: > https://patchew.org/QEMU/20230808115242.73025-1-akihiko.odaki@daynix.com/ Hmm doesn't that push the problem to real mmap() calls to a host system that doesn't support MAP_FIXED_NOREPLACE? I wonder if we need an internal flag rather than overloading the host flags? > >> >>> if (load_addr == -1) { >>> goto exit_mmap; >>
On 2023/08/08 22:48, Alex Bennée wrote: > > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> On 2023/08/08 18:43, Alex Bennée wrote: >>> Richard Henderson <richard.henderson@linaro.org> writes: >>> >>>> Use this as extra protection for the guest mapping over >>>> any qemu host mappings. >>>> >>>> Tested-by: Helge Deller <deller@gmx.de> >>>> Reviewed-by: Helge Deller <deller@gmx.de> >>>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>>> --- >>>> linux-user/elfload.c | 9 ++++++--- >>>> 1 file changed, 6 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c >>>> index 36e4026f05..1b4bb2d5af 100644 >>>> --- a/linux-user/elfload.c >>>> +++ b/linux-user/elfload.c >>>> @@ -3147,8 +3147,11 @@ static void load_elf_image(const char *image_name, int image_fd, >>>> /* >>>> * Reserve address space for all of this. >>>> * >>>> - * In the case of ET_EXEC, we supply MAP_FIXED so that we get >>>> - * exactly the address range that is required. >>>> + * In the case of ET_EXEC, we supply MAP_FIXED_NOREPLACE so that we get >>>> + * exactly the address range that is required. Without reserved_va, >>>> + * the guest address space is not isolated. We have attempted to avoid >>>> + * conflict with the host program itself via probe_guest_base, but using >>>> + * MAP_FIXED_NOREPLACE instead of MAP_FIXED provides an extra check. >>>> * >>>> * Otherwise this is ET_DYN, and we are searching for a location >>>> * that can hold the memory space required. If the image is >>>> @@ -3160,7 +3163,7 @@ static void load_elf_image(const char *image_name, int image_fd, >>>> */ >>>> 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), >>>> + (ehdr->e_type == ET_EXEC ? MAP_FIXED_NOREPLACE : 0), >>>> -1, 0); >>> We should probably also check the result == load_addr for the places >>> where MAP_FIXED_NOREPLACE isn't supported as we have this in osdep.h: >>> #ifndef MAP_FIXED_NOREPLACE >>> #define MAP_FIXED_NOREPLACE 0 >>> #endif >>> See 2667e069e7 (linux-user: don't use MAP_FIXED in >>> pgd_find_hole_fallback) >> >> It assumes target_mmap() emulates MAP_FIXED_NOREPLACE when the host >> does not support it as commit e69e032d1a ("linux-user: Use >> MAP_FIXED_NOREPLACE for do_brk()") already does, but defining >> MAP_FIXED_NOREPLACE zero breaks such emulation. I wrote a fix: >> https://patchew.org/QEMU/20230808115242.73025-1-akihiko.odaki@daynix.com/ > > Hmm doesn't that push the problem to real mmap() calls to a host system > that doesn't support MAP_FIXED_NOREPLACE? That can happen even without that patch if you run QEMU built with a new libc on old Linux so we should have prepared for that kind of situation. The man page also says: > Note that older kernels which do not recognize the MAP_FIXED_NOREPLACE > flag will typically (upon detecting a collision with a preexisting > mapping) fall back to a “non-MAP_FIXED” type of behavior: they will > return an address that is different from the requested address. > Therefore, backward-compatible software should check the returned > address against the requested address. https://man7.org/linux/man-pages/man2/mmap.2.html It basically means MAP_FIXED_NOREPLACE has no effect on a host that doesn't support it, and the existing code checking the returned address should continue to work. > > I wonder if we need an internal flag rather than overloading the host > flags? > >> >>> >>>> if (load_addr == -1) { >>>> goto exit_mmap; >>> > >
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2023/08/08 22:48, Alex Bennée wrote: >> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >> >>> On 2023/08/08 18:43, Alex Bennée wrote: >>>> Richard Henderson <richard.henderson@linaro.org> writes: >>>> >>>>> Use this as extra protection for the guest mapping over >>>>> any qemu host mappings. >>>>> >>>>> Tested-by: Helge Deller <deller@gmx.de> >>>>> Reviewed-by: Helge Deller <deller@gmx.de> >>>>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>>>> --- >>>>> linux-user/elfload.c | 9 ++++++--- >>>>> 1 file changed, 6 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c >>>>> index 36e4026f05..1b4bb2d5af 100644 >>>>> --- a/linux-user/elfload.c >>>>> +++ b/linux-user/elfload.c >>>>> @@ -3147,8 +3147,11 @@ static void load_elf_image(const char *image_name, int image_fd, >>>>> /* >>>>> * Reserve address space for all of this. >>>>> * >>>>> - * In the case of ET_EXEC, we supply MAP_FIXED so that we get >>>>> - * exactly the address range that is required. >>>>> + * In the case of ET_EXEC, we supply MAP_FIXED_NOREPLACE so that we get >>>>> + * exactly the address range that is required. Without reserved_va, >>>>> + * the guest address space is not isolated. We have attempted to avoid >>>>> + * conflict with the host program itself via probe_guest_base, but using >>>>> + * MAP_FIXED_NOREPLACE instead of MAP_FIXED provides an extra check. >>>>> * >>>>> * Otherwise this is ET_DYN, and we are searching for a location >>>>> * that can hold the memory space required. If the image is >>>>> @@ -3160,7 +3163,7 @@ static void load_elf_image(const char *image_name, int image_fd, >>>>> */ >>>>> 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), >>>>> + (ehdr->e_type == ET_EXEC ? MAP_FIXED_NOREPLACE : 0), >>>>> -1, 0); >>>> We should probably also check the result == load_addr for the places >>>> where MAP_FIXED_NOREPLACE isn't supported as we have this in osdep.h: >>>> #ifndef MAP_FIXED_NOREPLACE >>>> #define MAP_FIXED_NOREPLACE 0 >>>> #endif >>>> See 2667e069e7 (linux-user: don't use MAP_FIXED in >>>> pgd_find_hole_fallback) >>> >>> It assumes target_mmap() emulates MAP_FIXED_NOREPLACE when the host >>> does not support it as commit e69e032d1a ("linux-user: Use >>> MAP_FIXED_NOREPLACE for do_brk()") already does, but defining >>> MAP_FIXED_NOREPLACE zero breaks such emulation. I wrote a fix: >>> https://patchew.org/QEMU/20230808115242.73025-1-akihiko.odaki@daynix.com/ >> Hmm doesn't that push the problem to real mmap() calls to a host >> system >> that doesn't support MAP_FIXED_NOREPLACE? > > That can happen even without that patch if you run QEMU built with a > new libc on old Linux so we should have prepared for that kind of > situation. > > The man page also says: >> Note that older kernels which do not recognize the MAP_FIXED_NOREPLACE >> flag will typically (upon detecting a collision with a preexisting >> mapping) fall back to a “non-MAP_FIXED” type of behavior: they will >> return an address that is different from the requested address. >> Therefore, backward-compatible software should check the returned >> address against the requested address. > https://man7.org/linux/man-pages/man2/mmap.2.html > > It basically means MAP_FIXED_NOREPLACE has no effect on a host that > doesn't support it, and the existing code checking the returned > address should continue to work. OK - as long as it doesn't barf on unknown flags.
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 36e4026f05..1b4bb2d5af 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -3147,8 +3147,11 @@ static void load_elf_image(const char *image_name, int image_fd, /* * Reserve address space for all of this. * - * In the case of ET_EXEC, we supply MAP_FIXED so that we get - * exactly the address range that is required. + * In the case of ET_EXEC, we supply MAP_FIXED_NOREPLACE so that we get + * exactly the address range that is required. Without reserved_va, + * the guest address space is not isolated. We have attempted to avoid + * conflict with the host program itself via probe_guest_base, but using + * MAP_FIXED_NOREPLACE instead of MAP_FIXED provides an extra check. * * Otherwise this is ET_DYN, and we are searching for a location * that can hold the memory space required. If the image is @@ -3160,7 +3163,7 @@ static void load_elf_image(const char *image_name, int image_fd, */ 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), + (ehdr->e_type == ET_EXEC ? MAP_FIXED_NOREPLACE : 0), -1, 0); if (load_addr == -1) { goto exit_mmap;