[PATCH] firmware: ti_sci: fix the secure_hdr in do_xfer
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@ti.com ---
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@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, xfer->tx_message.len);
base-commit: a1448e3cf9a0602d284566d6cacf60b96c3c1316
On Jan 24, 2024 at 15:45:58 +0530, Dhruva Gole wrote:
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@ti.com
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@konsulko.com
drivers/firmware/ti_sci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Kindly ignore this patch, seems like it's from an older u-boot. I have rebased an sent new patch with updated boot logs.
Please forgive the spam.
Dhruva Gole d-gole@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@ti.com
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@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;
secure_hdr is pointer now, the value may be same but (struct ti_sci_secure_msg_hdr) would make more sense
Regards, Kamlesh
xfer->tx_message.len);base-commit: a1448e3cf9a0602d284566d6cacf60b96c3c1316
2.34.1
On Jan 24, 2024 at 16:42:12 +0530, Kamlesh Gurudasani wrote:
Dhruva Gole d-gole@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@ti.com
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@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;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.
On 18:38-20240124, Dhruva Gole wrote:
On Jan 24, 2024 at 16:42:12 +0530, Kamlesh Gurudasani wrote:
Dhruva Gole d-gole@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@ti.com
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@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;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?
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@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@ti.com
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@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;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.
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@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@ti.com
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@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;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.
https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/TISCI_heade... "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."
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.
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@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@ti.com
Boot tested for sanity on AM62x SK https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@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;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_heade... "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.
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@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@ti.com > --- > > Boot tested for sanity on AM62x SK > https://gist.github.com/DhruvaG2000/724ceba3a0db03f4b0bff47de1160074 > > Cc: Nishanth Menon nm@ti.com > Cc: Tom Rini trini@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_heade... "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.
On 10:55-20240125, Andrew Davis wrote:
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.
Chatting with manorit, the reason why we need the secure code is because of boot R5. boot R5 starts of in "secure mode" when it hands off from Boot ROM over to the Secondary bootloader. The initial set of calls we have to make unfortunately needs to be on secure pipe (think j721e load sysfw call, board config load call etc..) - which needs this support - which again, we should probably document in the code so that people don't go scratching our heads again.
That is unfortunately confusing enough for code since 99% of rest of u-boot flow does not use secure comm path.
participants (4)
-
Andrew Davis -
Dhruva Gole -
Kamlesh Gurudasani -
Nishanth Menon