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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>>
>> --- 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@xxxxxxxxxxxxxx>

Thanks.

Jan