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

[Andrew Davis <afd@ti.com>]

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.
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.

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);
>>>       }