arinc653: misc small adjustments

[PATCH v2 3/3] arinc653: avoid array overrun

Posted by Jan Beulich 1 week, 1 day ago

Incrementing ->sched_index between bounds check and array access may
result in accessing one past the array when that is fully filled
(->num_schedule_entries == ARINC653_MAX_DOMAINS_PER_SCHEDULE).

Fixes: 22787f2e107c ("ARINC 653 scheduler")
Reported-by: Kamil Frankowicz <kamil.frankowicz@cert.pl>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Jürgen, provided I understood him correctly, suggests that something like

    while ( now >= sched_priv->next_switch_time )
    {
        sched_priv->sched_index++;
        ASSERT(sched_priv->sched_index < sched_priv->num_schedule_entries);
        sched_priv->next_switch_time +=
            sched_priv->schedule[sched_priv->sched_index].runtime;
    }

should also be valid to move to, due to constraints applied by
arinc653_sched_set(). I'm hesitant to make such a change though, not
really knowing the scheduler; the change here looks more obviously correct
to me. Albeit the Fixes: tag may thus want dropping.
---
v2: New.

--- a/xen/common/sched/arinc653.c
+++ b/xen/common/sched/arinc653.c
@@ -552,12 +552,9 @@ a653sched_do_schedule(
 
     /* Switch minor frame or find correct minor frame after a miss */
     while ( (now >= sched_priv->next_switch_time) &&
-        (sched_priv->sched_index < sched_priv->num_schedule_entries) )
-    {
-        sched_priv->sched_index++;
+            (++sched_priv->sched_index < sched_priv->num_schedule_entries) )
         sched_priv->next_switch_time +=
             sched_priv->schedule[sched_priv->sched_index].runtime;
-    }
 
     /*
      * If we exhausted the domains in the schedule and still have time left

Re: [PATCH v2 3/3] arinc653: avoid array overrun

Posted by Jürgen Groß 1 week, 1 day ago

On 25.03.26 13:55, Jan Beulich wrote:
> Incrementing ->sched_index between bounds check and array access may
> result in accessing one past the array when that is fully filled
> (->num_schedule_entries == ARINC653_MAX_DOMAINS_PER_SCHEDULE).
> 
> Fixes: 22787f2e107c ("ARINC 653 scheduler")
> Reported-by: Kamil Frankowicz <kamil.frankowicz@cert.pl>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

with ...

> ---
> Jürgen, provided I understood him correctly, suggests that something like
> 
>      while ( now >= sched_priv->next_switch_time )
>      {
>          sched_priv->sched_index++;
>          ASSERT(sched_priv->sched_index < sched_priv->num_schedule_entries);
>          sched_priv->next_switch_time +=
>              sched_priv->schedule[sched_priv->sched_index].runtime;
>      }
> 
> should also be valid to move to, due to constraints applied by
> arinc653_sched_set(). I'm hesitant to make such a change though, not
> really knowing the scheduler; the change here looks more obviously correct
> to me. Albeit the Fixes: tag may thus want dropping.

the Fixes: tag dropped, as the constraints mentioned are IMO really enough
to avoid an issue.

I agree that the current code is far from obviously correct, so your patch
is definitively an improvement.


Juergen

Re: [PATCH v2 3/3] arinc653: avoid array overrun

Posted by Stewart Hildebrand 1 week, 1 day ago

On 3/25/26 09:22, Jürgen Groß wrote:
> On 25.03.26 13:55, Jan Beulich wrote:
>> Incrementing ->sched_index between bounds check and array access may
>> result in accessing one past the array when that is fully filled
>> (->num_schedule_entries == ARINC653_MAX_DOMAINS_PER_SCHEDULE).
>>
>> Fixes: 22787f2e107c ("ARINC 653 scheduler")
>> Reported-by: Kamil Frankowicz <kamil.frankowicz@cert.pl>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Stewart Hildebrand <stewart@stew.dk>

Thanks for this.

> 
> with ...
> 
>> ---
>> Jürgen, provided I understood him correctly, suggests that something like
>>
>>      while ( now >= sched_priv->next_switch_time )
>>      {
>>          sched_priv->sched_index++;
>>          ASSERT(sched_priv->sched_index < sched_priv->num_schedule_entries);
>>          sched_priv->next_switch_time +=
>>              sched_priv->schedule[sched_priv->sched_index].runtime;
>>      }
>>
>> should also be valid to move to, due to constraints applied by
>> arinc653_sched_set().

Not quite, because major_frame is allowed to be larger than the sum of the
runtimes, and in that case the ASSERT would trigger during the idle period.

>> I'm hesitant to make such a change though, not
>> really knowing the scheduler; the change here looks more obviously correct
>> to me. Albeit the Fixes: tag may thus want dropping.
> 
> the Fixes: tag dropped, as the constraints mentioned are IMO really enough
> to avoid an issue.

No, the constraints aren't enough, the out-of-bounds access would occur during
an idle period of a fully filled schedule. I suggest keeping the Fixes: tag.

> 
> I agree that the current code is far from obviously correct, so your patch
> is definitively an improvement.
> 
> 
> Juergen

Re: [PATCH v2 3/3] arinc653: avoid array overrun

Posted by Jan Beulich 1 week ago

On 25.03.2026 21:43, Stewart Hildebrand wrote:
> On 3/25/26 09:22, Jürgen Groß wrote:
>> On 25.03.26 13:55, Jan Beulich wrote:
>>> Incrementing ->sched_index between bounds check and array access may
>>> result in accessing one past the array when that is fully filled
>>> (->num_schedule_entries == ARINC653_MAX_DOMAINS_PER_SCHEDULE).
>>>
>>> Fixes: 22787f2e107c ("ARINC 653 scheduler")
>>> Reported-by: Kamil Frankowicz <kamil.frankowicz@cert.pl>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Stewart Hildebrand <stewart@stew.dk>
> 
> Thanks for this.
> 
>>
>> with ...
>>
>>> ---
>>> Jürgen, provided I understood him correctly, suggests that something like
>>>
>>>      while ( now >= sched_priv->next_switch_time )
>>>      {
>>>          sched_priv->sched_index++;
>>>          ASSERT(sched_priv->sched_index < sched_priv->num_schedule_entries);
>>>          sched_priv->next_switch_time +=
>>>              sched_priv->schedule[sched_priv->sched_index].runtime;
>>>      }
>>>
>>> should also be valid to move to, due to constraints applied by
>>> arinc653_sched_set().
> 
> Not quite, because major_frame is allowed to be larger than the sum of the
> runtimes, and in that case the ASSERT would trigger during the idle period.
> 
>>> I'm hesitant to make such a change though, not
>>> really knowing the scheduler; the change here looks more obviously correct
>>> to me. Albeit the Fixes: tag may thus want dropping.
>>
>> the Fixes: tag dropped, as the constraints mentioned are IMO really enough
>> to avoid an issue.
> 
> No, the constraints aren't enough, the out-of-bounds access would occur during
> an idle period of a fully filled schedule. I suggest keeping the Fixes: tag.

Jürgen, am I okay to keep your R-b with Fixes: re-added?

Jan

Re: [PATCH v2 3/3] arinc653: avoid array overrun

Posted by Jürgen Groß 1 week ago

On 26.03.26 08:37, Jan Beulich wrote:
> On 25.03.2026 21:43, Stewart Hildebrand wrote:
>> On 3/25/26 09:22, Jürgen Groß wrote:
>>> On 25.03.26 13:55, Jan Beulich wrote:
>>>> Incrementing ->sched_index between bounds check and array access may
>>>> result in accessing one past the array when that is fully filled
>>>> (->num_schedule_entries == ARINC653_MAX_DOMAINS_PER_SCHEDULE).
>>>>
>>>> Fixes: 22787f2e107c ("ARINC 653 scheduler")
>>>> Reported-by: Kamil Frankowicz <kamil.frankowicz@cert.pl>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>
>> Reviewed-by: Stewart Hildebrand <stewart@stew.dk>
>>
>> Thanks for this.
>>
>>>
>>> with ...
>>>
>>>> ---
>>>> Jürgen, provided I understood him correctly, suggests that something like
>>>>
>>>>       while ( now >= sched_priv->next_switch_time )
>>>>       {
>>>>           sched_priv->sched_index++;
>>>>           ASSERT(sched_priv->sched_index < sched_priv->num_schedule_entries);
>>>>           sched_priv->next_switch_time +=
>>>>               sched_priv->schedule[sched_priv->sched_index].runtime;
>>>>       }
>>>>
>>>> should also be valid to move to, due to constraints applied by
>>>> arinc653_sched_set().
>>
>> Not quite, because major_frame is allowed to be larger than the sum of the
>> runtimes, and in that case the ASSERT would trigger during the idle period.
>>
>>>> I'm hesitant to make such a change though, not
>>>> really knowing the scheduler; the change here looks more obviously correct
>>>> to me. Albeit the Fixes: tag may thus want dropping.
>>>
>>> the Fixes: tag dropped, as the constraints mentioned are IMO really enough
>>> to avoid an issue.
>>
>> No, the constraints aren't enough, the out-of-bounds access would occur during
>> an idle period of a fully filled schedule. I suggest keeping the Fixes: tag.
> 
> Jürgen, am I okay to keep your R-b with Fixes: re-added?

Yes, of course.


Juergen