diff mbox series

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

Message ID 20240124130758.237233-1-d-gole@ti.com
State Superseded
Delegated to: Tom Rini
Headers show
Series [V3] firmware: ti_sci: fix the secure_hdr in do_xfer | expand

Commit Message

Dhruva Gole Jan. 24, 2024, 1:07 p.m. UTC 
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(-)


base-commit: 15e7927b5a2d33666af19879577bf0c30ab088fe

Comments

Nishanth Menon Jan. 24, 2024, 6:09 p.m. UTC | #1 
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.
Dhruva Gole Jan. 29, 2024, 6:14 a.m. UTC | #2 
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?
Nishanth Menon Jan. 29, 2024, 12:20 p.m. UTC | #3 
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.
diff mbox series

Patch

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 */