ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

[PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by Chad Jablonski 1 month ago

Cast default_sc_bottom to uint32_t before shifting left to prevent
sign extension when assigning to uint64_t.

Fixes: CID 1645615
Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
---
 hw/display/ati.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 6cf243bcf9..f9773e1154 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
         val |= s->regs.default_tile << 16;
         break;
     case DEFAULT_SC_BOTTOM_RIGHT:
-        val = (s->regs.default_sc_bottom << 16) |
+        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
               s->regs.default_sc_right;
         break;
     case SC_TOP:
-- 
2.52.0

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by BALATON Zoltan 1 month ago

On Mon, 9 Mar 2026, Chad Jablonski wrote:
> Cast default_sc_bottom to uint32_t before shifting left to prevent
> sign extension when assigning to uint64_t.
>
> Fixes: CID 1645615
> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
> ---
> hw/display/ati.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 6cf243bcf9..f9773e1154 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
>         val |= s->regs.default_tile << 16;
>         break;
>     case DEFAULT_SC_BOTTOM_RIGHT:
> -        val = (s->regs.default_sc_bottom << 16) |
> +        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
>               s->regs.default_sc_right;
>         break;
>     case SC_TOP:

I think this is not needed. This is likely a false positive from Coverity 
because the valid bits for sc regs are bits 13:0 which we enforce on write 
so you should not get sign extension here. We typically don't do such 
fixes for these but mark them in Coverity but cc'd Peter Maydell who knows 
this better.

Regards,
BALATON Zoltan

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by Philippe Mathieu-Daudé 1 month ago

On 9/3/26 22:41, BALATON Zoltan wrote:
> On Mon, 9 Mar 2026, Chad Jablonski wrote:
>> Cast default_sc_bottom to uint32_t before shifting left to prevent
>> sign extension when assigning to uint64_t.
>>
>> Fixes: CID 1645615
>> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
>> ---
>> hw/display/ati.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index 6cf243bcf9..f9773e1154 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr 
>> addr, unsigned int size)
>>         val |= s->regs.default_tile << 16;
>>         break;
>>     case DEFAULT_SC_BOTTOM_RIGHT:
>> -        val = (s->regs.default_sc_bottom << 16) |
>> +        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
>>               s->regs.default_sc_right;

What about using deposit64() which is uncontroversial?

             val = deposit64(s->regs.default_sc_right, 16,
                             13, s->regs.default_sc_bottom);

>>         break;
>>     case SC_TOP:
> 
> I think this is not needed. This is likely a false positive from 
> Coverity because the valid bits for sc regs are bits 13:0 which we 
> enforce on write so you should not get sign extension here. We typically 
> don't do such fixes for these but mark them in Coverity but cc'd Peter 
> Maydell who knows this better.
> 
> Regards,
> BALATON Zoltan

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by BALATON Zoltan 1 month ago

On Mon, 9 Mar 2026, Philippe Mathieu-Daudé wrote:
> On 9/3/26 22:41, BALATON Zoltan wrote:
>> On Mon, 9 Mar 2026, Chad Jablonski wrote:
>>> Cast default_sc_bottom to uint32_t before shifting left to prevent
>>> sign extension when assigning to uint64_t.
>>> 
>>> Fixes: CID 1645615
>>> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
>>> ---
>>> hw/display/ati.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>>> index 6cf243bcf9..f9773e1154 100644
>>> --- a/hw/display/ati.c
>>> +++ b/hw/display/ati.c
>>> @@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
>>> unsigned int size)
>>>         val |= s->regs.default_tile << 16;
>>>         break;
>>>     case DEFAULT_SC_BOTTOM_RIGHT:
>>> -        val = (s->regs.default_sc_bottom << 16) |
>>> +        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
>>>               s->regs.default_sc_right;
>
> What about using deposit64() which is uncontroversial?
>
>            val = deposit64(s->regs.default_sc_right, 16,
>                            13, s->regs.default_sc_bottom);

I can't make sense of that without going and looking what the arguments of 
this function are so I'd rather keep it simple. As I said, the values here 
can't be large enough to sign extend and there may be other similar cases 
that are already marked in Coverity. But if we still want to fix all of 
them then maybe we could try to make val uint32_t then do something about 
it once at the return for all cases. That seems better than trying to 
handle in each case inconsistently. The simplest would still be to tell 
Converity it's wrong here.

Regards,
BALATON Zoltan

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by Philippe Mathieu-Daudé 1 month ago

On 9/3/26 23:59, BALATON Zoltan wrote:
> On Mon, 9 Mar 2026, Philippe Mathieu-Daudé wrote:
>> On 9/3/26 22:41, BALATON Zoltan wrote:
>>> On Mon, 9 Mar 2026, Chad Jablonski wrote:
>>>> Cast default_sc_bottom to uint32_t before shifting left to prevent
>>>> sign extension when assigning to uint64_t.
>>>>
>>>> Fixes: CID 1645615
>>>> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
>>>> ---
>>>> hw/display/ati.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>>>> index 6cf243bcf9..f9773e1154 100644
>>>> --- a/hw/display/ati.c
>>>> +++ b/hw/display/ati.c
>>>> @@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr 
>>>> addr, unsigned int size)
>>>>         val |= s->regs.default_tile << 16;
>>>>         break;
>>>>     case DEFAULT_SC_BOTTOM_RIGHT:
>>>> -        val = (s->regs.default_sc_bottom << 16) |
>>>> +        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
>>>>               s->regs.default_sc_right;
>>
>> What about using deposit64() which is uncontroversial?
>>
>>            val = deposit64(s->regs.default_sc_right, 16,
>>                            13, s->regs.default_sc_bottom);

(alternatively deposit32)

> I can't make sense of that without going and looking what the arguments 
> of this function are so I'd rather keep it simple. As I said, the values 
> here can't be large enough to sign extend and there may be other similar 
> cases that are already marked in Coverity. But if we still want to fix 
> all of them then maybe we could try to make val uint32_t then do 
> something about it once at the return for all cases. That seems better 
> than trying to handle in each case inconsistently. The simplest would 
> still be to tell Converity it's wrong here.

Until the next register is implemented and Coverity complains again,
and another round-up on the mailing list until Peter manually marks it.
I was hoping for a way to avoid all that hassle.

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by BALATON Zoltan 1 month ago

On Tue, 10 Mar 2026, Philippe Mathieu-Daudé wrote:
> On 9/3/26 23:59, BALATON Zoltan wrote:
>> On Mon, 9 Mar 2026, Philippe Mathieu-Daudé wrote:
>>> On 9/3/26 22:41, BALATON Zoltan wrote:
>>>> On Mon, 9 Mar 2026, Chad Jablonski wrote:
>>>>> Cast default_sc_bottom to uint32_t before shifting left to prevent
>>>>> sign extension when assigning to uint64_t.
>>>>> 
>>>>> Fixes: CID 1645615
>>>>> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
>>>>> ---
>>>>> hw/display/ati.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>>>>> index 6cf243bcf9..f9773e1154 100644
>>>>> --- a/hw/display/ati.c
>>>>> +++ b/hw/display/ati.c
>>>>> @@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr 
>>>>> addr, unsigned int size)
>>>>>         val |= s->regs.default_tile << 16;
>>>>>         break;
>>>>>     case DEFAULT_SC_BOTTOM_RIGHT:
>>>>> -        val = (s->regs.default_sc_bottom << 16) |
>>>>> +        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
>>>>>               s->regs.default_sc_right;
>>> 
>>> What about using deposit64() which is uncontroversial?
>>> 
>>>            val = deposit64(s->regs.default_sc_right, 16,
>>>                            13, s->regs.default_sc_bottom);
>
> (alternatively deposit32)
>
>> I can't make sense of that without going and looking what the arguments of 
>> this function are so I'd rather keep it simple. As I said, the values here 
>> can't be large enough to sign extend and there may be other similar cases 
>> that are already marked in Coverity. But if we still want to fix all of 
>> them then maybe we could try to make val uint32_t then do something about 
>> it once at the return for all cases. That seems better than trying to 
>> handle in each case inconsistently. The simplest would still be to tell 
>> Converity it's wrong here.
>
> Until the next register is implemented and Coverity complains again,
> and another round-up on the mailing list until Peter manually marks it.
> I was hoping for a way to avoid all that hassle.

Changing local val to uint32_t and cast it at the last return line should 
also solve this if the issue is assigning to uint64_t without a cast. To 
me that seems better than making changes to every case separately and 
would allow me to avoid additional complexity. If you think it would help 
I can make a patch but I can't test it.

Regards,
BALATON Zoltan

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by Philippe Mathieu-Daudé 1 month ago

On 10/3/26 13:47, BALATON Zoltan wrote:
> On Tue, 10 Mar 2026, Philippe Mathieu-Daudé wrote:
>> On 9/3/26 23:59, BALATON Zoltan wrote:
>>> On Mon, 9 Mar 2026, Philippe Mathieu-Daudé wrote:
>>>> On 9/3/26 22:41, BALATON Zoltan wrote:
>>>>> On Mon, 9 Mar 2026, Chad Jablonski wrote:
>>>>>> Cast default_sc_bottom to uint32_t before shifting left to prevent
>>>>>> sign extension when assigning to uint64_t.
>>>>>>
>>>>>> Fixes: CID 1645615
>>>>>> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
>>>>>> ---
>>>>>> hw/display/ati.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>>>>>> index 6cf243bcf9..f9773e1154 100644
>>>>>> --- a/hw/display/ati.c
>>>>>> +++ b/hw/display/ati.c
>>>>>> @@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, 
>>>>>> hwaddr addr, unsigned int size)
>>>>>>         val |= s->regs.default_tile << 16;
>>>>>>         break;
>>>>>>     case DEFAULT_SC_BOTTOM_RIGHT:
>>>>>> -        val = (s->regs.default_sc_bottom << 16) |
>>>>>> +        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
>>>>>>               s->regs.default_sc_right;
>>>>
>>>> What about using deposit64() which is uncontroversial?
>>>>
>>>>            val = deposit64(s->regs.default_sc_right, 16,
>>>>                            13, s->regs.default_sc_bottom);
>>
>> (alternatively deposit32)
>>
>>> I can't make sense of that without going and looking what the 
>>> arguments of this function are so I'd rather keep it simple. As I 
>>> said, the values here can't be large enough to sign extend and there 
>>> may be other similar cases that are already marked in Coverity. But 
>>> if we still want to fix all of them then maybe we could try to make 
>>> val uint32_t then do something about it once at the return for all 
>>> cases. That seems better than trying to handle in each case 
>>> inconsistently. The simplest would still be to tell Converity it's 
>>> wrong here.
>>
>> Until the next register is implemented and Coverity complains again,
>> and another round-up on the mailing list until Peter manually marks it.
>> I was hoping for a way to avoid all that hassle.
> 
> Changing local val to uint32_t and cast it at the last return line 
> should also solve this if the issue is assigning to uint64_t without a 
> cast. To me that seems better than making changes to every case 
> separately and would allow me to avoid additional complexity. If you 
> think it would help I can make a patch but I can't test it.

No more time / energy for this topic, so do what you think is best for
the project, I don't mind much anymore. I shouldn't have intervened at
all first, but since I merged this patch, I felt a bit concerned.
Actually the simplest is you mark that issue yourself on Coverity;
anybody can request an account for free. I'm done with this thread.

Thanks,

Phil.

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by BALATON Zoltan 1 month ago

On Tue, 10 Mar 2026, Philippe Mathieu-Daudé wrote:
> On 10/3/26 13:47, BALATON Zoltan wrote:
>> On Tue, 10 Mar 2026, Philippe Mathieu-Daudé wrote:
>>> On 9/3/26 23:59, BALATON Zoltan wrote:
>>>> On Mon, 9 Mar 2026, Philippe Mathieu-Daudé wrote:
>>>>> On 9/3/26 22:41, BALATON Zoltan wrote:
>>>>>> On Mon, 9 Mar 2026, Chad Jablonski wrote:
>>>>>>> Cast default_sc_bottom to uint32_t before shifting left to prevent
>>>>>>> sign extension when assigning to uint64_t.
>>>>>>> 
>>>>>>> Fixes: CID 1645615
>>>>>>> Signed-off-by: Chad Jablonski <chad@jablonski.xyz>
>>>>>>> ---
>>>>>>> hw/display/ati.c | 2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>> 
>>>>>>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>>>>>>> index 6cf243bcf9..f9773e1154 100644
>>>>>>> --- a/hw/display/ati.c
>>>>>>> +++ b/hw/display/ati.c
>>>>>>> @@ -514,7 +514,7 @@ static uint64_t ati_mm_read(void *opaque, hwaddr 
>>>>>>> addr, unsigned int size)
>>>>>>>         val |= s->regs.default_tile << 16;
>>>>>>>         break;
>>>>>>>     case DEFAULT_SC_BOTTOM_RIGHT:
>>>>>>> -        val = (s->regs.default_sc_bottom << 16) |
>>>>>>> +        val = ((uint32_t)s->regs.default_sc_bottom << 16) |
>>>>>>>               s->regs.default_sc_right;
>>>>> 
>>>>> What about using deposit64() which is uncontroversial?
>>>>> 
>>>>>            val = deposit64(s->regs.default_sc_right, 16,
>>>>>                            13, s->regs.default_sc_bottom);
>>> 
>>> (alternatively deposit32)
>>> 
>>>> I can't make sense of that without going and looking what the arguments 
>>>> of this function are so I'd rather keep it simple. As I said, the values 
>>>> here can't be large enough to sign extend and there may be other similar 
>>>> cases that are already marked in Coverity. But if we still want to fix 
>>>> all of them then maybe we could try to make val uint32_t then do 
>>>> something about it once at the return for all cases. That seems better 
>>>> than trying to handle in each case inconsistently. The simplest would 
>>>> still be to tell Converity it's wrong here.
>>> 
>>> Until the next register is implemented and Coverity complains again,
>>> and another round-up on the mailing list until Peter manually marks it.
>>> I was hoping for a way to avoid all that hassle.
>> 
>> Changing local val to uint32_t and cast it at the last return line should 
>> also solve this if the issue is assigning to uint64_t without a cast. To me 
>> that seems better than making changes to every case separately and would 
>> allow me to avoid additional complexity. If you think it would help I can 
>> make a patch but I can't test it.
>
> No more time / energy for this topic, so do what you think is best for
> the project, I don't mind much anymore. I shouldn't have intervened at
> all first, but since I merged this patch, I felt a bit concerned.
> Actually the simplest is you mark that issue yourself on Coverity;
> anybody can request an account for free. I'm done with this thread.

I did not know I can get access to Coverity. Then I can take a look there. 
I still think there's nothing to fix here because even if uint16_t 
default_sc_bottom is promoted to int for the calculation we don't support 
32 bit hosts any more so this can't get sign extension issue even in that 
case. So I think Converity is just confused here and we should not make 
code more complex for it.

Regards,
BALATON Zoltan

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by Peter Maydell 1 month ago

On Tue, 10 Mar 2026 at 14:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> I did not know I can get access to Coverity. Then I can take a look there.
> I still think there's nothing to fix here because even if uint16_t
> default_sc_bottom is promoted to int for the calculation we don't support
> 32 bit hosts any more so this can't get sign extension issue even in that
> case. So I think Converity is just confused here and we should not make
> code more complex for it.

None of the types here depend on whether the host is 32 bit or 64 bit:
 * default_sc_bottom is "uint16_t", which is always 16 bits unsigned
 * (default_sc_bottom << 16) is "int", which is 32-bit and signed
   on any platform we have ever cared about
 * val is "uint64_t", which is always 64 bits unsigned

So if default_sc_bottom bit 15 is set (0x8000) then we will end up
with an "int" value 0x8000_0000 (shift by 16 into the sign bit),
which then gets sign-extended to give a uint64_t
0xffff_ffff_8000_0000 when it is assigned to val.

Coverity warns about this because "I took an unsigned type
and shifted it right and assigned it to a larger unsigned type,
and in the middle of that it got handled as signed" is one
of C's unintuitive corners.

The thing that makes this specific case not a bug is that the value
of default_sc_bottom is always masked so that bit 15 is zero
(which is not something Coverity's analysis is capable of
spotting, because it's too far removed from the site of the
sign-extension).

We are about 50:50 on whether we prefer to mark these as
false-positives or else to insert a cast or other thing
to enforce that the intermediate type in the shift is
unsigned.

-- PMM

Re: [PATCH] ati-vga: Fix sign extension in DEFAULT_SC_BOTTOM_RIGHT read

Posted by BALATON Zoltan 1 month ago

On Tue, 10 Mar 2026, Peter Maydell wrote:
> On Tue, 10 Mar 2026 at 14:28, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> I did not know I can get access to Coverity. Then I can take a look there.
>> I still think there's nothing to fix here because even if uint16_t
>> default_sc_bottom is promoted to int for the calculation we don't support
>> 32 bit hosts any more so this can't get sign extension issue even in that
>> case. So I think Converity is just confused here and we should not make
>> code more complex for it.
>
> None of the types here depend on whether the host is 32 bit or 64 bit:
> * default_sc_bottom is "uint16_t", which is always 16 bits unsigned
> * (default_sc_bottom << 16) is "int", which is 32-bit and signed
>   on any platform we have ever cared about

Yes you're right this is what causes the warning here.

> * val is "uint64_t", which is always 64 bits unsigned
>
> So if default_sc_bottom bit 15 is set (0x8000) then we will end up
> with an "int" value 0x8000_0000 (shift by 16 into the sign bit),
> which then gets sign-extended to give a uint64_t
> 0xffff_ffff_8000_0000 when it is assigned to val.
>
> Coverity warns about this because "I took an unsigned type
> and shifted it right and assigned it to a larger unsigned type,
> and in the middle of that it got handled as signed" is one
> of C's unintuitive corners.
>
> The thing that makes this specific case not a bug is that the value
> of default_sc_bottom is always masked so that bit 15 is zero
> (which is not something Coverity's analysis is capable of
> spotting, because it's too far removed from the site of the
> sign-extension).
>
> We are about 50:50 on whether we prefer to mark these as
> false-positives or else to insert a cast or other thing
> to enforce that the intermediate type in the shift is
> unsigned.

If we want to fix it I think making val uint32_t would be better than a 
cast just in this one case as that would avoid all similar problems in 
other cases too, wouldn't it?

Regards,
BALATON Zoltan