[PATCH] firmware: ti_sci: fix the secure_hdr in do_xfer

[Dhruva Gole]

On Jan 24, 2024 at 12:08:13 -0600, Nishanth Menon wrote:
> On 23:07-20240124, Dhruva Gole wrote:
> > On Jan 24, 2024 at 10:39:10 -0600, Nishanth Menon wrote:
> > > On 18:38-20240124, Dhruva Gole wrote:
> > > > On Jan 24, 2024 at 16:42:12 +0530, Kamlesh Gurudasani wrote:
> > > > > Dhruva Gole <d-gole at ti.com> writes:
> > > > > 
> > > > > > The secure_hdr needs to be 0 init-ed however this was never being put
> > > > > > into the secure_buf, leading to possibility of the first 4 bytes of
> > > > > > secure_buf being possibly garbage.
> > > > > >
> > > > > > Fix this by initialising the secure_hdr itself to the secure_buf
> > > > > > location, thus when we make it 0, it automatically ensures the first 4
> > > > > > bytes are 0.
> > > > > >
> > > > > > Fixes: 32cd25128bd849 ("firmware: Add basic support for TI System Control Interface (TI SCI)")
> > > > > > Signed-off-by: Dhruva Gole <d-gole at ti.com>
> > > > > > ---
> > > > > >
> > > > > > Boot tested for sanity on AM62x SK
> > > > > > https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
> > > > > >
> > > > > > Cc: Nishanth Menon <nm at ti.com>
> > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > >
> > > > > >  drivers/firmware/ti_sci.c | 6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> > > > > > index f5f659c11274..83ee8401a731 100644
> > > > > > --- a/drivers/firmware/ti_sci.c
> > > > > > +++ b/drivers/firmware/ti_sci.c
> > > > > > @@ -236,13 +236,13 @@ static int ti_sci_do_xfer(struct ti_sci_info *info,
> > > > > >  {
> > > > > >  	struct k3_sec_proxy_msg *msg = &xfer->tx_message;
> > > > > >  	u8 secure_buf[info->desc->max_msg_size];
> > > > > > -	struct ti_sci_secure_msg_hdr secure_hdr;
> > > > > > +	struct ti_sci_secure_msg_hdr *secure_hdr = (struct ti_sci_secure_msg_hdr *)secure_buf;
> > > > > >  	int ret;
> > > > > >  
> > > > > >  	if (info->is_secure) {
> > > > > >  		/* ToDo: get checksum of the entire message */
> > > > > > -		secure_hdr.checksum = 0;
> > > > > > -		secure_hdr.reserved = 0;
> > > > > > +		secure_hdr->checksum = 0;
> > > > > > +		secure_hdr->reserved = 0;
> > > > > >  		memcpy(&secure_buf[sizeof(secure_hdr)],xfer->tx_message.buf,
> > > > > secure_hdr is pointer now, the value may be same but (struct
> > > > > ti_sci_secure_msg_hdr) would make more sense
> > > > 
> > > > Good catch Kamlesh! I have sent a new revision addressing this.
> > > > 
> > > 
> > > Makes no sense why we have secure API support in U-Boot. what is using
> > > this?
> > 
> > In my understanding of generic K3 boot flow, things like proc_boot and
> > even applying or removing of firewalls will need a secure channel to
> > talk to TIFS right? From my understanding secure host can only talk to
> > TIFS and make such requests hence secure API.
> 
> U-boot runs in NS world, you cant even talk to the Trustzone Sec proxy
> without triggering a firewall violation.

I am not going to debate on whether U-Boot is or isn't a secure entity.
The original code base was making secure_hdr.checksum = 0;

But this wasn't really being used anywhere. I am just making sure what the
original code base intended to do is being actually done.

> 
> 
> https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/TISCI_header.html?highlight=header#secure-messaging-header
> "The Secure Messaging Header is only required when sending messages over
> secure transport. Messages sent over non-secure transport must not
> contain the secure messaging header."

Yes, understood. I do not have the context behind the commit
32cd25128bd849 ("firmware: Add basic support for TI System Control
Interface (TI SCI)") but perhaps you do. So please help me understand
why there's this code:
memcpy(&secure_buf[sizeof(secure_hdr)], xfer->tx_message.buf, xfer->tx_message.len);

which is basically trying to create a 4 byte offset (trying to abide by the secure
msg format?) 
If UBoot as you say is non secure let's just send regular TISCI messages. Are you
suggesting that we trim all of if (info->is_secure) and related "secure"
code from uboot? Then that's a separate cleanup we need to have.


> 
> btw, that checksum field should be renamed integ_check, but anyways..
> 
> So, I do not see a reason to even have that if condition in the first
> place and what real bug was getting fixed in this patch.

"Real" bug or "potential" bug, the code did not seem to be doing what it should have
been doing, that's all. For anyone reading the checksum = 0 that is
there in today's code it's super confusing.

Either get rid of it completely, or else make it do what it was trying
to do. Let me know how we should proceed.


-- 
Best regards,
Dhruva Gole <d-gole at ti.com>