* [PATCH] linux-user: mprotect() should returns 0 when len is 0.
@ 2022-10-06 15:38 Soichiro Isshiki
2022-10-06 18:13 ` Peter Maydell
2022-10-07 0:38 ` Richard Henderson
0 siblings, 2 replies; 6+ messages in thread
From: Soichiro Isshiki @ 2022-10-06 15:38 UTC (permalink / raw)
To: qemu-devel, qemu-trivial; +Cc: sisshiki1969
From: sisshiki1969 <sisshiki@mac.com>
For now, qemu-x86_64 returns ENOMEM when mprotect() was called with an argument
len is 0 from a guest process.
This behavior is incompatible with the current Linux implementation,
which mprotect() with len = 0 does nothing and returns 0,
although it does not appear to be explicitly described in man.
This is due to the following function which always returns false if len = 0.
```C
static inline bool guest_range_valid_untagged(abi_ulong start, abi_ulong len)
{
return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len + 1;
}
```
This patch fix this incompatibility problem.
Signed-off-by: sisshiki1969 <sisshiki@mac.com>
---
linux-user/mmap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 28f3bc85ed..1ed79459ea 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -130,12 +130,12 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
}
len = TARGET_PAGE_ALIGN(len);
end = start + len;
- if (!guest_range_valid_untagged(start, len)) {
- return -TARGET_ENOMEM;
- }
if (len == 0) {
return 0;
}
+ if (!guest_range_valid_untagged(start, len)) {
+ return -TARGET_ENOMEM;
+ }
mmap_lock();
host_start = start & qemu_host_page_mask;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] linux-user: mprotect() should returns 0 when len is 0.
2022-10-06 15:38 [PATCH] linux-user: mprotect() should returns 0 when len is 0 Soichiro Isshiki
@ 2022-10-06 18:13 ` Peter Maydell
2022-10-06 18:31 ` Richard Henderson
2022-10-07 0:38 ` Richard Henderson
1 sibling, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2022-10-06 18:13 UTC (permalink / raw)
To: Soichiro Isshiki
Cc: qemu-devel, qemu-trivial, sisshiki1969, Richard Henderson
On Thu, 6 Oct 2022 at 19:05, Soichiro Isshiki
<sisshiki@isshiki-clinic.com> wrote:
>
> From: sisshiki1969 <sisshiki@mac.com>
>
> For now, qemu-x86_64 returns ENOMEM when mprotect() was called with an argument
> len is 0 from a guest process.
> This behavior is incompatible with the current Linux implementation,
> which mprotect() with len = 0 does nothing and returns 0,
> although it does not appear to be explicitly described in man.
>
> This is due to the following function which always returns false if len = 0.
>
> ```C
> static inline bool guest_range_valid_untagged(abi_ulong start, abi_ulong len)
> {
> return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len + 1;
> }
>
> ```
>
> This patch fix this incompatibility problem.
>
> Signed-off-by: sisshiki1969 <sisshiki@mac.com>
> ---
> linux-user/mmap.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 28f3bc85ed..1ed79459ea 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -130,12 +130,12 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
> }
> len = TARGET_PAGE_ALIGN(len);
> end = start + len;
> - if (!guest_range_valid_untagged(start, len)) {
> - return -TARGET_ENOMEM;
> - }
> if (len == 0) {
> return 0;
> }
> + if (!guest_range_valid_untagged(start, len)) {
> + return -TARGET_ENOMEM;
> + }
>
> mmap_lock();
> host_start = start & qemu_host_page_mask;
Cc'ing Richard -- is this the right fix, or would it be better instead
to make guest_range_valid_untagged() correctly handle a zero-length
range ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] linux-user: mprotect() should returns 0 when len is 0.
2022-10-06 18:13 ` Peter Maydell
@ 2022-10-06 18:31 ` Richard Henderson
2022-10-06 22:28 ` 一色聡一郎
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2022-10-06 18:31 UTC (permalink / raw)
To: Peter Maydell, Soichiro Isshiki; +Cc: qemu-devel, qemu-trivial, sisshiki1969
On 10/6/22 11:13, Peter Maydell wrote:
> On Thu, 6 Oct 2022 at 19:05, Soichiro Isshiki
> <sisshiki@isshiki-clinic.com> wrote:
>>
>> From: sisshiki1969 <sisshiki@mac.com>
>>
>> For now, qemu-x86_64 returns ENOMEM when mprotect() was called with an argument
>> len is 0 from a guest process.
>> This behavior is incompatible with the current Linux implementation,
>> which mprotect() with len = 0 does nothing and returns 0,
>> although it does not appear to be explicitly described in man.
>>
>> This is due to the following function which always returns false if len = 0.
>>
>> ```C
>> static inline bool guest_range_valid_untagged(abi_ulong start, abi_ulong len)
>> {
>> return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len + 1;
>> }
>>
...
> Cc'ing Richard -- is this the right fix, or would it be better instead
> to make guest_range_valid_untagged() correctly handle a zero-length
> range ?
I think fixing the range check might be best.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] linux-user: mprotect() should returns 0 when len is 0.
2022-10-06 18:31 ` Richard Henderson
@ 2022-10-06 22:28 ` 一色聡一郎
0 siblings, 0 replies; 6+ messages in thread
From: 一色聡一郎 @ 2022-10-06 22:28 UTC (permalink / raw)
To: Richard Henderson; +Cc: Peter Maydell, qemu-devel, qemu-trivial, sisshiki1969
[-- Attachment #1: Type: text/plain, Size: 2437 bytes --]
Thank you for your response.
Yes, we can also modify guest_range_valid_untagged() like this:
static inline bool guest_range_valid_untagged(abi_ulong start, abi_ulong
len)
{
- return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len + 1;
+ return !len || len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX -
len + 1;
}
But actually, guest_range_valid_untagged() is called from several sites
other than target_mprotect().
(1) target_mmap() in bsd-user
(2) target_madvise() in linux-user
(3) target_mmap() in linux-user
(4) target_munmap() in linux-user
(5) access_ok_untagged() in linux-user/qemu.h
(6) target_mremap() in linux-user
(7) do_shmat() in linux-user/syscall.c
(1)-(5) have explicit guards for the condition of len = 0 in front of
calling guest_range_valid_untagged().
(1) https://gitlab.com/qemu-project/qemu/-/blob/master/bsd-user/mmap.c#L477
(2)
https://gitlab.com/qemu-project/qemu/-/blob/master/linux-user/mmap.c#L900
(3)
https://gitlab.com/qemu-project/qemu/-/blob/master/linux-user/mmap.c#L456
(4)
https://gitlab.com/qemu-project/qemu/-/blob/master/linux-user/mmap.c#L724
(5)
https://gitlab.com/qemu-project/qemu/-/blob/master/linux-user/qemu.h#L176
But I'm not sure whether this change is correct for (6) and (7).
2022年10月7日(金) 3:31 Richard Henderson <richard.henderson@linaro.org>:
> On 10/6/22 11:13, Peter Maydell wrote:
> > On Thu, 6 Oct 2022 at 19:05, Soichiro Isshiki
> > <sisshiki@isshiki-clinic.com> wrote:
> >>
> >> From: sisshiki1969 <sisshiki@mac.com>
> >>
> >> For now, qemu-x86_64 returns ENOMEM when mprotect() was called with an
> argument
> >> len is 0 from a guest process.
> >> This behavior is incompatible with the current Linux implementation,
> >> which mprotect() with len = 0 does nothing and returns 0,
> >> although it does not appear to be explicitly described in man.
> >>
> >> This is due to the following function which always returns false if len
> = 0.
> >>
> >> ```C
> >> static inline bool guest_range_valid_untagged(abi_ulong start,
> abi_ulong len)
> >> {
> >> return len - 1 <= GUEST_ADDR_MAX && start <= GUEST_ADDR_MAX - len
> + 1;
> >> }
> >>
> ...
> > Cc'ing Richard -- is this the right fix, or would it be better instead
> > to make guest_range_valid_untagged() correctly handle a zero-length
> > range ?
>
> I think fixing the range check might be best.
>
>
> r~
>
[-- Attachment #2: Type: text/html, Size: 3967 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] linux-user: mprotect() should returns 0 when len is 0.
2022-10-06 15:38 [PATCH] linux-user: mprotect() should returns 0 when len is 0 Soichiro Isshiki
2022-10-06 18:13 ` Peter Maydell
@ 2022-10-07 0:38 ` Richard Henderson
1 sibling, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2022-10-07 0:38 UTC (permalink / raw)
To: Soichiro Isshiki, qemu-devel, qemu-trivial; +Cc: sisshiki1969
On 10/6/22 08:38, Soichiro Isshiki wrote:
> From: sisshiki1969 <sisshiki@mac.com>
>
> For now, qemu-x86_64 returns ENOMEM when mprotect() was called with an argument
> len is 0 from a guest process.
> This behavior is incompatible with the current Linux implementation,
> which mprotect() with len = 0 does nothing and returns 0,
> although it does not appear to be explicitly described in man.
You're right that the ordering of checks differs from the kernel.
The kernel has:
(1) validate prot !(growdown && growup)
(2) validate page aligned
(3) pass len == 0
(4) validate no wraparound
(5) validate prot for arch.
(6) validate vma valid.
while we have
(1) validate page aligned
(2) validate prot for arch
(3) validate vma valid
(4) pass len == 0.
My previous answer vs guest_range_valid_untagged is incorrect considering all of this: if
start > GUEST_ADDR_MAX, that *should* fail vma valid, but the kernel will have returned
success before that.
Although, sorta, this smells like a kernel bug.
Why should mprotect(-4096, 0, 0) succeed while mprotect(-4096, 4096, 0) fails?
But anyway, if we're going to fix len == 0 to match, we might as well fix all 3 test
ordering bugs at the same time.
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] linux-user: mprotect() should returns 0 when len is 0.
@ 2022-10-06 16:43 Soichiro Isshiki
0 siblings, 0 replies; 6+ messages in thread
From: Soichiro Isshiki @ 2022-10-06 16:43 UTC (permalink / raw)
Cc: sisshiki1969, Laurent Vivier, open list:All patches CC here
From: sisshiki1969 <sisshiki@mac.com>
Signed-off-by: sisshiki1969 <sisshiki@mac.com>
---
linux-user/mmap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 28f3bc85ed..1ed79459ea 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -130,12 +130,12 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
}
len = TARGET_PAGE_ALIGN(len);
end = start + len;
- if (!guest_range_valid_untagged(start, len)) {
- return -TARGET_ENOMEM;
- }
if (len == 0) {
return 0;
}
+ if (!guest_range_valid_untagged(start, len)) {
+ return -TARGET_ENOMEM;
+ }
mmap_lock();
host_start = start & qemu_host_page_mask;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-07 0:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 15:38 [PATCH] linux-user: mprotect() should returns 0 when len is 0 Soichiro Isshiki
2022-10-06 18:13 ` Peter Maydell
2022-10-06 18:31 ` Richard Henderson
2022-10-06 22:28 ` 一色聡一郎
2022-10-07 0:38 ` Richard Henderson
2022-10-06 16:43 Soichiro Isshiki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.