In commit 8c5931de0ac7738809 we added support for SVE extended
sigframe records.  These mean that the signal frame might now be
larger than the size of the target_rt_sigframe record, so make sure
we call lock_user on the entire frame size when we're creating it.
(The code for restoring the signal frame already correctly handles
the extended records by locking the 'extra' section separately to the
main section.)

In particular, this fixes a bug even for non-SVE signal frames,
because it extends the locked section to cover the
target_rt_frame_record. Previously this was part of 'struct
target_rt_sigframe', but in commit e1eecd1d9d4c1ade3 we pulled
it out into its own struct, and so locking the target_rt_sigframe
alone doesn't cover it. This bug would mean that we would fail
to correctly handle the case where a signal was taken with
SP pointing 16 bytes into an unwritable page, with the page
immediately below it in memory being writable.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The requirements to trigger the bug sound implausible, except
that the stack page might be unwritable because we just
executed some trampoline code from it, so perhaps not so
unlikely as it first seems? Not sure whether to put into 2.12
or not...
---
 linux-user/signal.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index e6dfe0adfd..b283270391 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1858,7 +1858,8 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
 
     frame_addr = get_sigframe(ka, env, layout.total_size);
     trace_user_setup_frame(env, frame_addr);
-    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
+    frame = lock_user(VERIFY_WRITE, frame_addr, layout.total_size, 0);
+    if (!frame) {
         goto give_sigsegv;
     }
 
@@ -1904,11 +1905,11 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
         env->xregs[2] = frame_addr + offsetof(struct target_rt_sigframe, uc);
     }
 
-    unlock_user_struct(frame, frame_addr, 1);
+    unlock_user(frame, frame_addr, layout.total_size);
     return;
 
  give_sigsegv:
-    unlock_user_struct(frame, frame_addr, 1);
+    unlock_user(frame, frame_addr, layout.total_size);
     force_sigsegv(usig);
 }
 
-- 
2.17.0

On 16 April 2018 at 16:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> In commit 8c5931de0ac7738809 we added support for SVE extended
> sigframe records.  These mean that the signal frame might now be
> larger than the size of the target_rt_sigframe record, so make sure
> we call lock_user on the entire frame size when we're creating it.
> (The code for restoring the signal frame already correctly handles
> the extended records by locking the 'extra' section separately to the
> main section.)
>
> In particular, this fixes a bug even for non-SVE signal frames,
> because it extends the locked section to cover the
> target_rt_frame_record. Previously this was part of 'struct
> target_rt_sigframe', but in commit e1eecd1d9d4c1ade3 we pulled
> it out into its own struct, and so locking the target_rt_sigframe
> alone doesn't cover it. This bug would mean that we would fail
> to correctly handle the case where a signal was taken with
> SP pointing 16 bytes into an unwritable page, with the page
> immediately below it in memory being writable.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The requirements to trigger the bug sound implausible, except
> that the stack page might be unwritable because we just
> executed some trampoline code from it, so perhaps not so
> unlikely as it first seems? Not sure whether to put into 2.12
> or not...
> ---

I initially thought this was a non-bug with SVE not enabled, but
when I was testing it I realized it enlarged the size of the
region we VERIFY_WRITE even in the normal case, after commit
aac8f55633f4470e.

-- PMM

On 04/16/2018 05:19 AM, Peter Maydell wrote:
> In commit 8c5931de0ac7738809 we added support for SVE extended
> sigframe records.  These mean that the signal frame might now be
> larger than the size of the target_rt_sigframe record, so make sure
> we call lock_user on the entire frame size when we're creating it.
> (The code for restoring the signal frame already correctly handles
> the extended records by locking the 'extra' section separately to the
> main section.)
> 
> In particular, this fixes a bug even for non-SVE signal frames,
> because it extends the locked section to cover the
> target_rt_frame_record. Previously this was part of 'struct
> target_rt_sigframe', but in commit e1eecd1d9d4c1ade3 we pulled
> it out into its own struct, and so locking the target_rt_sigframe
> alone doesn't cover it. This bug would mean that we would fail
> to correctly handle the case where a signal was taken with
> SP pointing 16 bytes into an unwritable page, with the page
> immediately below it in memory being writable.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The requirements to trigger the bug sound implausible, except
> that the stack page might be unwritable because we just
> executed some trampoline code from it, so perhaps not so
> unlikely as it first seems? Not sure whether to put into 2.12
> or not...
> ---
>  linux-user/signal.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Yes, let's just go ahead and fix this all properly now.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

On 16 April 2018 at 20:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 04/16/2018 05:19 AM, Peter Maydell wrote:
>> In commit 8c5931de0ac7738809 we added support for SVE extended
>> sigframe records.  These mean that the signal frame might now be
>> larger than the size of the target_rt_sigframe record, so make sure
>> we call lock_user on the entire frame size when we're creating it.
>> (The code for restoring the signal frame already correctly handles
>> the extended records by locking the 'extra' section separately to the
>> main section.)
>>
>> In particular, this fixes a bug even for non-SVE signal frames,
>> because it extends the locked section to cover the
>> target_rt_frame_record. Previously this was part of 'struct
>> target_rt_sigframe', but in commit e1eecd1d9d4c1ade3 we pulled
>> it out into its own struct, and so locking the target_rt_sigframe
>> alone doesn't cover it. This bug would mean that we would fail
>> to correctly handle the case where a signal was taken with
>> SP pointing 16 bytes into an unwritable page, with the page
>> immediately below it in memory being writable.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> The requirements to trigger the bug sound implausible, except
>> that the stack page might be unwritable because we just
>> executed some trampoline code from it, so perhaps not so
>> unlikely as it first seems? Not sure whether to put into 2.12
>> or not...
>> ---
>>  linux-user/signal.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> Yes, let's just go ahead and fix this all properly now.
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Applied, thanks.

-- PMM