diff mbox series

[v4,37/45] linux-user/aarch64: Do not allow duplicate or short sve records

Message ID 20220628042117.368549-38-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Scalable Matrix Extension | expand

Commit Message

Richard Henderson June 28, 2022, 4:21 a.m. UTC 
In parse_user_sigframe, the kernel rejects duplicate sve records,
or records that are smaller than the header.  We were silently
allowing these cases to pass, dropping the record.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/aarch64/signal.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Peter Maydell July 4, 2022, 12:08 p.m. UTC | #1 
On Tue, 28 Jun 2022 at 05:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> In parse_user_sigframe, the kernel rejects duplicate sve records,
> or records that are smaller than the header.  We were silently
> allowing these cases to pass, dropping the record.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/aarch64/signal.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
> index 8b352abb97..8fbe98d72f 100644
> --- a/linux-user/aarch64/signal.c
> +++ b/linux-user/aarch64/signal.c
> @@ -318,10 +318,13 @@ static int target_restore_sigframe(CPUARMState *env,
>              break;
>
>          case TARGET_SVE_MAGIC:
> +            if (sve || size < sizeof(struct target_sve_context)) {
> +                goto err;
> +            }
>              if (cpu_isar_feature(aa64_sve, env_archcpu(env))) {
>                  vq = sve_vq(env);
>                  sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
> -                if (!sve && size == sve_size) {
> +                if (size == sve_size) {
>                      sve = (struct target_sve_context *)ctx;
>                      break;
>                  }

On the other hand, the kernel seems to happily allow records
which are larger than the SVE_SIG_CONTEXT_SIZE, whereas we
ignore the record unless there's an exact size match.

I notice the kernel has a bunch of signal frame test
cases in  tools/testing/selftests/arm64/signal/testcases --
do we pass those ?

thanks
-- PMM
Richard Henderson July 5, 2022, 3:27 a.m. UTC | #2 
On 7/4/22 17:38, Peter Maydell wrote:
> I notice the kernel has a bunch of signal frame test
> cases in  tools/testing/selftests/arm64/signal/testcases --
> do we pass those ?

Most but not all of them.  The ones we don't pass are those for which SVE state has been 
discarded across a syscall and so the signal frame record is expected to be missing.  I 
thought about fixing those, but decided not to do within this series.


r~
Richard Henderson July 5, 2022, 3:30 a.m. UTC | #3 
On 7/4/22 17:38, Peter Maydell wrote:
>>           case TARGET_SVE_MAGIC:
>> +            if (sve || size < sizeof(struct target_sve_context)) {
>> +                goto err;
>> +            }
>>               if (cpu_isar_feature(aa64_sve, env_archcpu(env))) {
>>                   vq = sve_vq(env);
>>                   sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
>> -                if (!sve && size == sve_size) {
>> +                if (size == sve_size) {
>>                       sve = (struct target_sve_context *)ctx;
>>                       break;
>>                   }
> 
> On the other hand, the kernel seems to happily allow records
> which are larger than the SVE_SIG_CONTEXT_SIZE, whereas we
> ignore the record unless there's an exact size match.

Yeah, this gets fixed properly in patch 39.
Perhaps I should simply squash this with that?


r~
Richard Henderson July 5, 2022, 3:32 a.m. UTC | #4 
On 7/5/22 09:00, Richard Henderson wrote:
> On 7/4/22 17:38, Peter Maydell wrote:
>>>           case TARGET_SVE_MAGIC:
>>> +            if (sve || size < sizeof(struct target_sve_context)) {
>>> +                goto err;
>>> +            }
>>>               if (cpu_isar_feature(aa64_sve, env_archcpu(env))) {
>>>                   vq = sve_vq(env);
>>>                   sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
>>> -                if (!sve && size == sve_size) {
>>> +                if (size == sve_size) {
>>>                       sve = (struct target_sve_context *)ctx;
>>>                       break;
>>>                   }
>>
>> On the other hand, the kernel seems to happily allow records
>> which are larger than the SVE_SIG_CONTEXT_SIZE, whereas we
>> ignore the record unless there's an exact size match.
> 
> Yeah, this gets fixed properly in patch 39.
> Perhaps I should simply squash this with that?

Bah!  No, those are two separate checks: the minimum size to contain vq and flags 
(target_sve_context) and the minimum size to contain all of the vector data 
(TARGET_SVE_SIG_CONTEXT_SIZE).

The latter *is* fixed in patch 39, but this one should stay as-is.


r~
diff mbox series

Patch

diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
index 8b352abb97..8fbe98d72f 100644
--- a/linux-user/aarch64/signal.c
+++ b/linux-user/aarch64/signal.c
@@ -318,10 +318,13 @@  static int target_restore_sigframe(CPUARMState *env,
             break;
 
         case TARGET_SVE_MAGIC:
+            if (sve || size < sizeof(struct target_sve_context)) {
+                goto err;
+            }
             if (cpu_isar_feature(aa64_sve, env_archcpu(env))) {
                 vq = sve_vq(env);
                 sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
-                if (!sve && size == sve_size) {
+                if (size == sve_size) {
                     sve = (struct target_sve_context *)ctx;
                     break;
                 }