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

[Andrew Davis]

On 1/24/24 11:43 PM, Dhruva Gole wrote:
> 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:

A little background, this checksum was envisioned to help protect
messages from glitches in-flight for security/safety reasons. For
whatever reason it was only ever added for messages sent over
"secure" channels. Which means the message sent changes based
on what channel you use to send the message (which is a bit of a
layering violation IMHO but anyway..).

In TFA/OP-TEE which *can* send messages over secure channels we
append this header in the transport layer (secure proxy driver),
not here in the message protocol driver. If we decide to keep this
secure header code then it should be moved into the transport driver
(mailbox/k3-sec-proxy.c) as only it would know what channel the
message will be sent on (and so if it should have the header).

I'd say since U-Boot today cannot send over secure channels anyway,
lets just drop this code and if we ever need it then we add it back
over in the right spot at that time.

Andrew

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