Re: [PATCH 2/2] mailbox: ti-msgmgr: Read exact number of trailing message bytes

["Padhi, Beleswar" <b-padhi@ti.com>]

On 9/30/2026 9:08 PM, Andrew Davis wrote:
> On 9/30/26 10:11 AM, Padhi, Beleswar wrote:
>>
>> On 9/30/2026 8:34 PM, Andrew Davis wrote:
>>> On 9/30/26 1:37 AM, Padhi, Beleswar wrote:
>>>>
>>>> On 9/30/2026 9:43 AM, Vignesh Raghavendra wrote:
>>>>>
>>>>> On 30/09/26 01:31, Beleswar Padhi wrote:
>>>>>> ti_msgmgr_send_data() copies the message to the data registers in 
>>>>>> u32
>>>>>> units. This has been harmless so far because the clients (TI-SCI)
>>>>>> always passed its preallocated message buffer, which is sized to the
>>>>>> maximum message size (60 or 64 bytes, a multiple of 4), so the extra
>>>>>> bytes read were still within the buffer. But, with a later TI-SCI
>>>>>> update, the message size can be varying now and not guaranteed to 
>>>>>> be a
>>>>>> multiple of 4, which would trigger KASAN out-of-bounds reports.
>>>>>>
>>>>>> Therefore, Build the trailing word by reading one byte at a time, 
>>>>>> only
>>>>>> up to the message length. The value written to the register is
>>>>>> unchanged.
>>>>>>
>>>>>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>>>>> This looks like a bug fixes and needs a Fixes: tag?
>>>>
>>>>
>>>> All mbox clients of ti-msgmgr currently send 60/64 byte sized 
>>>> messages. So this
>>>> is not a bug today. It is only a preparatory patch for future mbox 
>>>> clients which
>>>> can send message sizes like 19 bytes etc.
>>>>
>>>
>>> It's still a bug, even if no one happened to run into it yet.
>>
>>
>> So should every pointer be checked against NULL before dereferencing
>> in all drivers by that logic then?
>>
>
> I get what you are saying, but in many cases if passing NULL is
> allowed or not is an implied contract between caller and callee.
> In this case the caller explicitly states the message length to
> read, and the function doesn't respect that value. Which is a more
> clear case of broken contract, and therefor a bug IMHO.


Ah, the beauty. Thank you for providing this reasoning. Being able to
back your arguments with something so concrete and correct is
something I really respect about you.

Thanks,
Beleswar

>
> Andrew
>
>>> Probably good to add the Fixes tag anyway, just in case something
>>> else every gets backported also that needs the non-4-byte aligned
>>> reads.
>>
>>
>> Fair, makes sense. Will add Fixes tag in v3.
>>
>> Thanks,
>> Beleswar
>>
>>>
>>> Andrew
>>>
>>>> Thanks,
>>>> Beleswar
>>>>
>>>>>
>>>>>> ---
>>>>>>   drivers/mailbox/ti-msgmgr.c | 16 ++++++++++++----
>>>>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mailbox/ti-msgmgr.c 
>>>>>> b/drivers/mailbox/ti-msgmgr.c
>>>>>> index 425d5f9d9d0e3..44457a896510f 100644
>>>>>> --- a/drivers/mailbox/ti-msgmgr.c
>>>>>> +++ b/drivers/mailbox/ti-msgmgr.c
>>>>>> @@ -425,10 +425,18 @@ static int ti_msgmgr_send_data(struct 
>>>>>> mbox_chan *chan, void *data)
>>>>>>       trail_bytes = message->len % sizeof(u32);
>>>>>>       if (trail_bytes) {
>>>>>> -        u32 data_trail = *word_data;
>>>>>> -
>>>>>> -        /* Ensure all unused data is 0 */
>>>>>> -        data_trail &= 0xFFFFFFFF >> (8 * (sizeof(u32) - 
>>>>>> trail_bytes));
>>>>>> +        /*
>>>>>> +         * Read the trailing bytes one at a time instead of as a 
>>>>>> full
>>>>>> +         * u32, as the message buffer may end right after them 
>>>>>> and a
>>>>>> +         * u32 read would go past the end of it. This also 
>>>>>> leaves all
>>>>>> +         * unused data as 0.
>>>>>> +         */
>>>>>> +        u8 *byte_data = (u8 *)word_data;
>>>>>> +        u32 data_trail = 0;
>>>>>> +        int i;
>>>>>> +
>>>>>> +        for (i = 0; i < trail_bytes; i++)
>>>>>> +            data_trail |= byte_data[i] << (8 * i);
>>>>>>           writel(data_trail, data_reg);
>>>>>>           data_reg += sizeof(u32);
>>>>>>       }
>>>
>