Re: [PATCH RFC 08/15] arm_mpam: Fix ris_idx type to prevent range check bypass on truncation

[Yin Li <yin.li@oss.qualcomm.com>]

On 9/3/2026 9:27 PM, Andre Przywara wrote:
> Hi,
> 
> On 9/3/26 11:42, Yin Li wrote:
>>
>>
>> On 9/3/2026 12:22 AM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 8/11/26 15:30, Yin Li wrote:
>>>> The RIS index is read from device tree as u64 via 
>>>> of_property_read_reg(),
>>>
>>> what does it do that using an u64, actually? Do you refer to the reg 
>>> property of the ris subnode, which has a limit of 0xf in the DT 
>>> binding? So shouldn't it be an u8 all along, and we fix the types up 
>>> at the sources, rather than widening everything needlessly to u64?
>>>
>>
>> Hi Andre,
>>
>> Thanks for the review.
>>
>> Yes, this is the reg property of the ris subnode. The reason it starts
>> as u64 is that it's read via of_property_read_reg(), whose API takes a
> 
> Yes, I figured as much, *after* hitting the Send button ;-)
> 

😜

>> u64* for the value — so ris_idx has to be u64 at that point, regardless
>> of the 0xf limit in the binding.
> 
> Which actually makes me wonder whether this is the right function to 
> use, since there would be no translation (as indeed guaranteed by this 
> function), but also no size, and I guess no cell size requirements 
> beyond 1. I think it has the added benefit of checking #address-cells 
> and #size-cells, but technically a standard of_property_read_u32() would 
> do as well? Though this probably has the same problem, just with u32 ...
> 
>> If ris_idx were narrowed to u8 before reaching the range check in
>> mpam_ris_create_locked() (ris_idx >= MPAM_MSC_MAX_NUM_RIS), an
>> out-of-range value such as 0x100 would be truncated to 0x00 and silently
>> bypass that check. Keeping the wider type through the chain lets that
>> check see the real value and reject invalid indices.
>>
>> If you feel an explicit check right after of_property_read_reg() (with
>> the downstream types kept as u8) is cleaner, I'm glad to go that way —
>> whichever you prefer.
> 
> Yeah, I feel it's sane to already check the limit directly after parsing 
> from the DT, not only in mpam_ris_create() later. Do you know of any 
> particular reason this is done so late?
> If there is none, I think the cleanest is to keep of_property_read_reg() 
> and check against the limit already in that function. Then we can use a 
> u8 all along.
> 

Hi Andre,

No particular reason for the late check — it just followed the existing
structure, where mpam_ris_create() already does the range check for both
the ACPI and DT paths. Nothing requires it to happen there.

And agreed on keeping of_property_read_reg(): the #address-cells /
#size-cells validation is worth having, and of_property_read_u32()
wouldn't avoid the truncation anyway.

I'll check the limit right after of_property_read_reg() and use u8
throughout the downstream path. Checking before the narrowing avoids the
truncation concern at the source, and drops the needless widening. Will
do this in the next version.

> Cheers,
> Andre
> 
>>>> but was narrowed to u32 when passed to mpam_dt_parse_resource() and
>>>> further to u8 when passed to mpam_ris_create(). A value exceeding
>>>> MPAM_MSC_MAX_NUM_RIS could be silently truncated to a small index that
>>>> passes the range check in mpam_ris_create_locked(), leading to 
>>>> incorrect
>>>> RIS creation.
>>>>
>>>> Widen the ris_idx parameter through mpam_dt_parse_resource(),
>>>> mpam_ris_create_locked(), and mpam_ris_create() to u64 so the value
>>>> is preserved until the range check in mpam_ris_create_locked() rejects
>>>> out-of-range indices.
>>>>
>>>> Signed-off-by: Yin Li <yin.li@oss.qualcomm.com>
>>>> ---
>>>>   drivers/resctrl/mpam_devices.c | 6 +++---
>>>>   include/linux/arm_mpam.h       | 4 ++--
>>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/ 
>>>> mpam_devices.c
>>>> index cc9fa1d78925..1e082fb60e30 100644
>>>> --- a/drivers/resctrl/mpam_devices.c
>>>> +++ b/drivers/resctrl/mpam_devices.c
>>>> @@ -260,7 +260,7 @@ static int mpam_dt_count_msc(void)
>>>>   }
>>>>   static int mpam_dt_parse_resource(struct mpam_msc *msc, struct 
>>>> device_node *np,
>>>> -                  u32 ris_idx)
>>>> +                  u64 ris_idx)
>>>>   {
>>>>       int err = 0;
>>>>       u32 class_id = 0;
>>>> @@ -712,7 +712,7 @@ static int mpam_ris_get_affinity(struct mpam_msc 
>>>> *msc, cpumask_t *affinity,
>>>>       return 0;
>>>>   }
>>>> -static int mpam_ris_create_locked(struct mpam_msc *msc, u8 ris_idx,
>>>> +static int mpam_ris_create_locked(struct mpam_msc *msc, u64 ris_idx,
>>>>                     enum mpam_class_types type, u8 class_id,
>>>>                     int component_id)
>>>>   {
>>>> @@ -799,7 +799,7 @@ static void mpam_ris_destroy(struct mpam_msc_ris 
>>>> *ris)
>>>>           mpam_vmsc_destroy(vmsc);
>>>>   }
>>>> -int mpam_ris_create(struct mpam_msc *msc, u8 ris_idx,
>>>> +int mpam_ris_create(struct mpam_msc *msc, u64 ris_idx,
>>>>               enum mpam_class_types type, u8 class_id, int 
>>>> component_id)
>>>>   {
>>>>       int err;
>>>> diff --git a/include/linux/arm_mpam.h b/include/linux/arm_mpam.h
>>>> index f92a36187a52..30461cd71199 100644
>>>> --- a/include/linux/arm_mpam.h
>>>> +++ b/include/linux/arm_mpam.h
>>>> @@ -39,10 +39,10 @@ static inline int acpi_mpam_count_msc(void) 
>>>> { return -EINVAL; }
>>>>   #endif
>>>>   #ifdef CONFIG_ARM64_MPAM_DRIVER
>>>> -int mpam_ris_create(struct mpam_msc *msc, u8 ris_idx,
>>>> +int mpam_ris_create(struct mpam_msc *msc, u64 ris_idx,
>>>>               enum mpam_class_types type, u8 class_id, int 
>>>> component_id);
>>>>   #else
>>>> -static inline int mpam_ris_create(struct mpam_msc *msc, u8 ris_idx,
>>>> +static inline int mpam_ris_create(struct mpam_msc *msc, u64 ris_idx,
>>>>                     enum mpam_class_types type, u8 class_id,
>>>>                     int component_id)
>>>>   {
>>>>
>>>
>>
> 

-- 
Thx and BRs,
Yin