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

[Padhi, Beleswar]

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