[PATCH V3] 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
Changelog: v2 --> v3 Address Kamlesh's comment on v2: use sizeof(struct ti_sci_secure_msg_hdr)
v1 --> v2: Rebased on top of latest master branch
Cc: Nishanth Menon nm@ti.com Cc: Tom Rini trini@konsulko.com Cc: Neha Francis n-francis@ti.com Cc: Manorit Chawdhry m-chawdhry@ti.com Cc: Vignesh Raghavendra vigneshr@ti.com Cc: Kamlesh Gurudasani kamlesh@ti.com
---
drivers/firmware/ti_sci.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c index 6e9f93e9a302..49d2696a6d09 100644 --- a/drivers/firmware/ti_sci.c +++ b/drivers/firmware/ti_sci.c @@ -236,21 +236,21 @@ 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; - memcpy(&secure_buf[sizeof(secure_hdr)], xfer->tx_message.buf, + secure_hdr->checksum = 0; + secure_hdr->reserved = 0; + memcpy(&secure_buf[sizeof(struct ti_sci_secure_msg_hdr)], xfer->tx_message.buf, xfer->tx_message.len);
xfer->tx_message.buf = (u32 *)secure_buf; - xfer->tx_message.len += sizeof(secure_hdr); + xfer->tx_message.len += sizeof(struct ti_sci_secure_msg_hdr);
if (xfer->rx_len) - xfer->rx_len += sizeof(secure_hdr); + xfer->rx_len += sizeof(struct ti_sci_secure_msg_hdr); }
/* Send the message */
base-commit: 15e7927b5a2d33666af19879577bf0c30ab088fe
On 18:37-20240124, 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
Changelog: v2 --> v3 Address Kamlesh's comment on v2: use sizeof(struct ti_sci_secure_msg_hdr)
Lets finish discussing in: https://lore.kernel.org/all/20240124163910.sp7gt56lihoujm7k@etching/
Responding to keep patchworks happy.
On Jan 24, 2024 at 12:09:06 -0600, Nishanth Menon wrote:
On 18:37-20240124, 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
Changelog: v2 --> v3 Address Kamlesh's comment on v2: use sizeof(struct ti_sci_secure_msg_hdr)
Lets finish discussing in: https://lore.kernel.org/all/20240124163910.sp7gt56lihoujm7k@etching/
Bringing the conversation back to this latest patch revision, Based on where we left off: https://lore.kernel.org/all/20240125171335.qoxphnemadkh7xjd@gullible/
Would it be better to add a comment above ``if (info->is_secure) {`` in drivers/firmware/ti_sci.c as follows: The secure path will be used by R5 SPL bcause it starts of in "secure mode" when it hands off from Boot ROM over to the Secondary bootloader.
Kindly advise if the patch needs respin with that minor change, and if the rest of it seems okay?
On 11:44-20240129, Dhruva Gole wrote:
On Jan 24, 2024 at 12:09:06 -0600, Nishanth Menon wrote:
On 18:37-20240124, 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
Changelog: v2 --> v3 Address Kamlesh's comment on v2: use sizeof(struct ti_sci_secure_msg_hdr)
Lets finish discussing in: https://lore.kernel.org/all/20240124163910.sp7gt56lihoujm7k@etching/
Bringing the conversation back to this latest patch revision, Based on where we left off: https://lore.kernel.org/all/20240125171335.qoxphnemadkh7xjd@gullible/
Would it be better to add a comment above ``if (info->is_secure) {`` in drivers/firmware/ti_sci.c as follows: The secure path will be used by R5 SPL bcause it starts of in "secure mode" when it hands off from Boot ROM over to the Secondary bootloader.
Kindly advise if the patch needs respin with that minor change, and if the rest of it seems okay?
improve the commit message and add a documentation patch to add information around if (info->is_secure) please, so that we don't yet again need to dig up history.
participants (2)
-
Dhruva Gole -
Nishanth Menon