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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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@xxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

Reviewed-by: Stewart Hildebrand <stewart@xxxxxxx>

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