[PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[agrecascino123]

From: "Catherine A. Frederick" <chocola@animebitch.es>

---
 tcg/ppc/tcg-target.inc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index ee1f9227c1..a5450a5e67 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg dst, TCGReg src)
 
 static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 32) ? 32 : c;
     tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
 }
 
 static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 64) ? 64 : c;
     tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
 }
 
 static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 32) ? 32 : c;
     tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
 }
 
 static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 64) ? 64 : c;
     tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
 }
 
-- 
2.26.2

Re: [PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[Catherine A. Frederick / mptcultist]

Oh dear, it appears that git send-email ate the formatting, hang on.

On Wed, Jun 3, 2020 at 12:47 AM <agrecascino123@gmail.com> wrote:
>
> From: "Catherine A. Frederick" <chocola@animebitch.es>
>
> ---
>  tcg/ppc/tcg-target.inc.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
> index ee1f9227c1..a5450a5e67 100644
> --- a/tcg/ppc/tcg-target.inc.c
> +++ b/tcg/ppc/tcg-target.inc.c
> @@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg dst, TCGReg src)
>
>  static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 32) ? 32 : c;
>      tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
>  }
>
>  static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 64) ? 64 : c;
>      tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
>  }
>
>  static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 32) ? 32 : c;
>      tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
>  }
>
>  static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 64) ? 64 : c;
>      tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
>  }
>
> --
> 2.26.2
>

Re: [PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200603044701.10748-1-agrecascino123@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200603044701.10748-1-agrecascino123@gmail.com
Subject: [PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
d097b53 tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 25 lines checked

Commit d097b531df67 (tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200603044701.10748-1-agrecascino123@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

[PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[agrecascino123]

From: "Catherine A. Frederick" <chocola@animebitch.es>

Signed-off-by: "Catherine A. Frederick" <chocola@animebitch.es>
---
 tcg/ppc/tcg-target.inc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index ee1f9227c1..a5450a5e67 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg dst, TCGReg src)
 
 static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 32) ? 32 : c;
     tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
 }
 
 static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 64) ? 64 : c;
     tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
 }
 
 static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 32) ? 32 : c;
     tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
 }
 
 static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int c)
 {
+    c = ((unsigned)c > 64) ? 64 : c;
     tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
 }
 
-- 
2.26.2

Re: [PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[Philippe Mathieu-Daudé]

Hi Catherine,

On 6/3/20 7:23 AM, agrecascino123@gmail.com wrote:
> From: "Catherine A. Frederick" <chocola@animebitch.es>
> 
> Signed-off-by: "Catherine A. Frederick" <chocola@animebitch.es>
> ---
>  tcg/ppc/tcg-target.inc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
> index ee1f9227c1..a5450a5e67 100644
> --- a/tcg/ppc/tcg-target.inc.c
> +++ b/tcg/ppc/tcg-target.inc.c
> @@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg dst, TCGReg src)
>  
>  static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 32) ? 32 : c;
>      tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
>  }
>  
>  static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 64) ? 64 : c;
>      tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
>  }
>  
>  static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 32) ? 32 : c;
>      tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
>  }
>  
>  static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int c)
>  {
> +    c = ((unsigned)c > 64) ? 64 : c;
>      tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
>  }

I agree there is a bug, but I am not sure we should silently cap the
value this way. I'd rather see the caller provide a value in range, and
maybe the callee use 'tcg_debug_assert(c <= RANGE);' to catch future new
caller added missing the range check.

Re: [PATCH] tcg: Sanitize shift constants on ppc64le so that shift operations with large constants don't generate invalid instructions.

[Richard Henderson]

On 6/2/20 11:43 PM, Philippe Mathieu-Daudé wrote:
> 
> Hi Catherine,
> 
> On 6/3/20 7:23 AM, agrecascino123@gmail.com wrote:
>> From: "Catherine A. Frederick" <chocola@animebitch.es>
>>
>> Signed-off-by: "Catherine A. Frederick" <chocola@animebitch.es>
>> ---
>>  tcg/ppc/tcg-target.inc.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
>> index ee1f9227c1..a5450a5e67 100644
>> --- a/tcg/ppc/tcg-target.inc.c
>> +++ b/tcg/ppc/tcg-target.inc.c
>> @@ -790,21 +790,25 @@ static inline void tcg_out_ext32u(TCGContext *s, TCGReg dst, TCGReg src)
>>  
>>  static inline void tcg_out_shli32(TCGContext *s, TCGReg dst, TCGReg src, int c)
>>  {
>> +    c = ((unsigned)c > 32) ? 32 : c;
>>      tcg_out_rlw(s, RLWINM, dst, src, c, 0, 31 - c);
>>  }
>>  
>>  static inline void tcg_out_shli64(TCGContext *s, TCGReg dst, TCGReg src, int c)
>>  {
>> +    c = ((unsigned)c > 64) ? 64 : c;
>>      tcg_out_rld(s, RLDICR, dst, src, c, 63 - c);
>>  }
>>  
>>  static inline void tcg_out_shri32(TCGContext *s, TCGReg dst, TCGReg src, int c)
>>  {
>> +    c = ((unsigned)c > 32) ? 32 : c;
>>      tcg_out_rlw(s, RLWINM, dst, src, 32 - c, c, 31);
>>  }
>>  
>>  static inline void tcg_out_shri64(TCGContext *s, TCGReg dst, TCGReg src, int c)
>>  {
>> +    c = ((unsigned)c > 64) ? 64 : c;
>>      tcg_out_rld(s, RLDICL, dst, src, 64 - c, c);
>>  }
> 
> I agree there is a bug, but I am not sure we should silently cap the
> value this way. I'd rather see the caller provide a value in range, and
> maybe the callee use 'tcg_debug_assert(c <= RANGE);' to catch future new
> caller added missing the range check.

We have done this before: see 1fd95946657.

In tcg/README, we note that out-of-range shifts produce undefined results, but
should not trap with illegal instruction.

I would like to know more about where these out-of-range shifts are being
generated, but I do know that there are innocent ways by which this can happen.

For instance, one way in which we can translate a guest in which out-of-range
shifts produce zero is

  x = (shift < 32 ? y << shift : 0)

using INDEX_op_movcond_i32 for the ?: operator.  Which means that
we use the original (out-of-range) shift and subsequently discard the undefined
result.

Catherine, I think it would be more appropriate to mask C rather than bound it
to another out-of-range value: c &= 31 or c &= 64, with a comment about
avoiding an illegal instruction, just as in the tcg/sparc patch I reference above.


r~