[PATCH] x86/vRTC: don't overrun array when storing century field

[PATCH] x86/vRTC: don't overrun array when storing century field

[Jan Beulich]

rtc_ioport_write() has two writes of the new value, yet only one was made
aware of the century going outside of the array. Fold both writes by
changing the RTC_SET short-circuiting.

Fixes: f2ff80877f66 ("x86/vRTC: support century field")
Coverity ID: 1700943
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -521,20 +521,22 @@ static int rtc_ioport_write(RTCState *s,
     case RTC_MONTH:
     case RTC_YEAR:
     case RTC_CENTURY:
-        /* if in set mode, just write the register */
-        if ( (s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
-            s->hw.cmos_data[s->hw.cmos_index] = data;
-        else
+        /* If in set mode, just write the register. */
+        if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
         {
             /* Fetch the current time and update just this field. */
             s->current_tm = gmtime(get_localtime(d));
             rtc_copy_date(s);
-            if ( s->hw.cmos_index != RTC_CENTURY )
-                s->hw.cmos_data[s->hw.cmos_index] = data;
-            else
-                s->hw.century = data;
-            rtc_set_time(s);
         }
+
+        if ( s->hw.cmos_index != RTC_CENTURY )
+            s->hw.cmos_data[s->hw.cmos_index] = data;
+        else
+            s->hw.century = data;
+
+        if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
+            rtc_set_time(s);
+
         alarm_timer_update(s);
         break;
     case RTC_REG_A:

Re: [PATCH] x86/vRTC: don't overrun array when storing century field

[Roger Pau Monné]

On Mon, Sep 07, 2026 at 10:13:10AM +0200, Jan Beulich wrote:
> rtc_ioport_write() has two writes of the new value, yet only one was made
> aware of the century going outside of the array. Fold both writes by
> changing the RTC_SET short-circuiting.
> 
> Fixes: f2ff80877f66 ("x86/vRTC: support century field")
> Coverity ID: 1700943
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -521,20 +521,22 @@ static int rtc_ioport_write(RTCState *s,
>      case RTC_MONTH:
>      case RTC_YEAR:
>      case RTC_CENTURY:
> -        /* if in set mode, just write the register */
> -        if ( (s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
> -            s->hw.cmos_data[s->hw.cmos_index] = data;
> -        else
> +        /* If in set mode, just write the register. */
> +        if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
>          {
>              /* Fetch the current time and update just this field. */
>              s->current_tm = gmtime(get_localtime(d));
>              rtc_copy_date(s);
> -            if ( s->hw.cmos_index != RTC_CENTURY )
> -                s->hw.cmos_data[s->hw.cmos_index] = data;
> -            else
> -                s->hw.century = data;
> -            rtc_set_time(s);
>          }
> +
> +        if ( s->hw.cmos_index != RTC_CENTURY )
> +            s->hw.cmos_data[s->hw.cmos_index] = data;
> +        else
> +            s->hw.century = data;

Might it be best to do this based on the array size?  ie:

if ( s->hw.cmos_index < ARRAY_SIZE(s->hw.cmos_data) )
    s->hw.cmos_data[s->hw.cmos_index] = data;
else
{
    ASSERT(s->hw.cmos_index == RTC_CENTURY);
    s->hw.century = data;
}

I don't think we are going to use more indexes, but otherwise we could
use a switch.  In any case, this is a fix so I don't intend to delay
it any longer, with either the current code or the suggested array
size checking (if suitable):

Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks, Roger.

Re: [PATCH] x86/vRTC: don't overrun array when storing century field

[Jan Beulich]

On 07.09.2026 10:56, Roger Pau Monné wrote:
> On Mon, Sep 07, 2026 at 10:13:10AM +0200, Jan Beulich wrote:
>> rtc_ioport_write() has two writes of the new value, yet only one was made
>> aware of the century going outside of the array. Fold both writes by
>> changing the RTC_SET short-circuiting.
>>
>> Fixes: f2ff80877f66 ("x86/vRTC: support century field")
>> Coverity ID: 1700943
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/rtc.c
>> +++ b/xen/arch/x86/hvm/rtc.c
>> @@ -521,20 +521,22 @@ static int rtc_ioport_write(RTCState *s,
>>      case RTC_MONTH:
>>      case RTC_YEAR:
>>      case RTC_CENTURY:
>> -        /* if in set mode, just write the register */
>> -        if ( (s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
>> -            s->hw.cmos_data[s->hw.cmos_index] = data;
>> -        else
>> +        /* If in set mode, just write the register. */
>> +        if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
>>          {
>>              /* Fetch the current time and update just this field. */
>>              s->current_tm = gmtime(get_localtime(d));
>>              rtc_copy_date(s);
>> -            if ( s->hw.cmos_index != RTC_CENTURY )
>> -                s->hw.cmos_data[s->hw.cmos_index] = data;
>> -            else
>> -                s->hw.century = data;
>> -            rtc_set_time(s);
>>          }
>> +
>> +        if ( s->hw.cmos_index != RTC_CENTURY )
>> +            s->hw.cmos_data[s->hw.cmos_index] = data;
>> +        else
>> +            s->hw.century = data;
> 
> Might it be best to do this based on the array size?  ie:
> 
> if ( s->hw.cmos_index < ARRAY_SIZE(s->hw.cmos_data) )
>     s->hw.cmos_data[s->hw.cmos_index] = data;
> else
> {
>     ASSERT(s->hw.cmos_index == RTC_CENTURY);
>     s->hw.century = data;
> }

We could do so, but then consistently (i.e. also in rtc_ioport_read()).

> I don't think we are going to use more indexes, but otherwise we could
> use a switch.

We will want to gain further indexes, for alarm day/month (as indicated
in a remark in the original patch'es submission).

I decided (in the original patch) against switch() because they're a
little odd to have inside a case block already covering the same
(strictly speaking: a subset) of the cases. But once the other two
fields are added, I think switch() will be the form to use.

>  In any case, this is a fix so I don't intend to delay
> it any longer, with either the current code or the suggested array
> size checking (if suitable):
> 
> Acked-by: Roger Pau Monné <roger@xenproject.org>

Thanks.

Jan