target/s390x: Fix the accumulation of ccm in op_icm

[PATCH] target/s390x: Fix the accumulation of ccm in op_icm

Posted by Richard Henderson 3 years, 10 months ago

Coverity rightly reports that 0xff << pos can overflow.
This would affect the ICMH instruction.

Fixes: Coverity CID 1487161
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 5acfc0ff9b..ea7baf0832 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2622,7 +2622,7 @@ static DisasJumpType op_icm(DisasContext *s, DisasOps *o)
                 tcg_gen_qemu_ld8u(tmp, o->in2, get_mem_index(s));
                 tcg_gen_addi_i64(o->in2, o->in2, 1);
                 tcg_gen_deposit_i64(o->out, o->out, tmp, pos, 8);
-                ccm |= 0xff << pos;
+                ccm |= 0xffull << pos;
             }
             m3 = (m3 << 1) & 0xf;
             pos -= 8;
-- 
2.25.1

Re: [PATCH] target/s390x: Fix the accumulation of ccm in op_icm

Posted by Richard Henderson 3 years, 9 months ago

On 4/1/22 12:36, Richard Henderson wrote:
> Coverity rightly reports that 0xff << pos can overflow.
> This would affect the ICMH instruction.
> 
> Fixes: Coverity CID 1487161
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/tcg/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 5acfc0ff9b..ea7baf0832 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2622,7 +2622,7 @@ static DisasJumpType op_icm(DisasContext *s, DisasOps *o)
>                   tcg_gen_qemu_ld8u(tmp, o->in2, get_mem_index(s));
>                   tcg_gen_addi_i64(o->in2, o->in2, 1);
>                   tcg_gen_deposit_i64(o->out, o->out, tmp, pos, 8);
> -                ccm |= 0xff << pos;
> +                ccm |= 0xffull << pos;
>               }
>               m3 = (m3 << 1) & 0xf;
>               pos -= 8;

Queuing to tcg-next.


r~

Re: [PATCH] target/s390x: Fix the accumulation of ccm in op_icm

Posted by David Hildenbrand 3 years, 10 months ago

On 01.04.22 21:36, Richard Henderson wrote:
> Coverity rightly reports that 0xff << pos can overflow.
> This would affect the ICMH instruction.
> 
> Fixes: Coverity CID 1487161
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/tcg/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 5acfc0ff9b..ea7baf0832 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2622,7 +2622,7 @@ static DisasJumpType op_icm(DisasContext *s, DisasOps *o)
>                  tcg_gen_qemu_ld8u(tmp, o->in2, get_mem_index(s));
>                  tcg_gen_addi_i64(o->in2, o->in2, 1);
>                  tcg_gen_deposit_i64(o->out, o->out, tmp, pos, 8);
> -                ccm |= 0xff << pos;
> +                ccm |= 0xffull << pos;
>              }
>              m3 = (m3 << 1) & 0xf;
>              pos -= 8;

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH] target/s390x: Fix the accumulation of ccm in op_icm

Posted by Thomas Huth 3 years, 10 months ago

On 01/04/2022 21.36, Richard Henderson wrote:
> Coverity rightly reports that 0xff << pos can overflow.
> This would affect the ICMH instruction.
> 
> Fixes: Coverity CID 1487161
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/tcg/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 5acfc0ff9b..ea7baf0832 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2622,7 +2622,7 @@ static DisasJumpType op_icm(DisasContext *s, DisasOps *o)
>                   tcg_gen_qemu_ld8u(tmp, o->in2, get_mem_index(s));
>                   tcg_gen_addi_i64(o->in2, o->in2, 1);
>                   tcg_gen_deposit_i64(o->out, o->out, tmp, pos, 8);
> -                ccm |= 0xff << pos;
> +                ccm |= 0xffull << pos;
>               }
>               m3 = (m3 << 1) & 0xf;
>               pos -= 8;

Reviewed-by: Thomas Huth <thuth@redhat.com>

Is this still something for 7.0, or can it wait for the 7.1 cycle?

Re: [PATCH] target/s390x: Fix the accumulation of ccm in op_icm

Posted by Richard Henderson 3 years, 10 months ago

On 4/2/22 02:38, Thomas Huth wrote:
> On 01/04/2022 21.36, Richard Henderson wrote:
>> Coverity rightly reports that 0xff << pos can overflow.
>> This would affect the ICMH instruction.
>>
>> Fixes: Coverity CID 1487161
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/s390x/tcg/translate.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
>> index 5acfc0ff9b..ea7baf0832 100644
>> --- a/target/s390x/tcg/translate.c
>> +++ b/target/s390x/tcg/translate.c
>> @@ -2622,7 +2622,7 @@ static DisasJumpType op_icm(DisasContext *s, DisasOps *o)
>>                   tcg_gen_qemu_ld8u(tmp, o->in2, get_mem_index(s));
>>                   tcg_gen_addi_i64(o->in2, o->in2, 1);
>>                   tcg_gen_deposit_i64(o->out, o->out, tmp, pos, 8);
>> -                ccm |= 0xff << pos;
>> +                ccm |= 0xffull << pos;
>>               }
>>               m3 = (m3 << 1) & 0xf;
>>               pos -= 8;
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Is this still something for 7.0, or can it wait for the 7.1 cycle?

The bug has been present since 2012, affecting only the cc value of icmh.  It could wait 
for 7.1.

r~