[PATCH v1] firmware: thead: Fix buffer overflow and use standard endian macros

[PATCH v1] firmware: thead: Fix buffer overflow and use standard endian macros

[Michal Wilczynski]

Addresses two issues in the TH1520 AON firmware protocol driver:

1. Fix a potential buffer overflow where the code used unsafe pointer
   arithmetic to access the 'mode' field through the 'resource' pointer
   with an offset. This was flagged by Smatch static checker as:
   "buffer overflow 'data' 2 <= 3"

2. Replace custom RPC_SET_BE* and RPC_GET_BE* macros with standard
   kernel endianness conversion macros (cpu_to_be16, etc.) for better
   portability and maintainability.

The functionality was re-tested with the GPU power-up sequence,
confirming the GPU powers up correctly and the driver probes
successfully.

[   12.702370] powervr ffef400000.gpu: [drm] loaded firmware
powervr/rogue_36.52.104.182_v1.fw
[   12.711043] powervr ffef400000.gpu: [drm] FW version v1.0 (build
6645434 OS)
[   12.719787] [drm] Initialized powervr 1.0.0 for ffef400000.gpu on
minor 0

Fixes: e4b3cbd840e5 ("firmware: thead: Add AON firmware protocol driver")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/17a0ccce-060b-4b9d-a3c4-8d5d5823b1c9@stanley.mountain/
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/firmware/thead,th1520-aon.c           |  7 +-
 .../linux/firmware/thead/thead,th1520-aon.h   | 74 -------------------
 2 files changed, 3 insertions(+), 78 deletions(-)

diff --git a/drivers/firmware/thead,th1520-aon.c b/drivers/firmware/thead,th1520-aon.c
index 38f812ac9920..b87d4e8235b1 100644
--- a/drivers/firmware/thead,th1520-aon.c
+++ b/drivers/firmware/thead,th1520-aon.c
@@ -170,10 +170,9 @@ int th1520_aon_power_update(struct th1520_aon_chan *aon_chan, u16 rsrc,
 	hdr->func = TH1520_AON_PM_FUNC_SET_RESOURCE_POWER_MODE;
 	hdr->size = TH1520_AON_RPC_MSG_NUM;
 
-	RPC_SET_BE16(&msg.resource, 0, rsrc);
-	RPC_SET_BE16(&msg.resource, 2,
-		     (power_on ? TH1520_AON_PM_PW_MODE_ON :
-				 TH1520_AON_PM_PW_MODE_OFF));
+	msg.resource = cpu_to_be16(rsrc);
+	msg.mode = cpu_to_be16(power_on ? TH1520_AON_PM_PW_MODE_ON :
+					  TH1520_AON_PM_PW_MODE_OFF);
 
 	ret = th1520_aon_call_rpc(aon_chan, &msg);
 	if (ret)
diff --git a/include/linux/firmware/thead/thead,th1520-aon.h b/include/linux/firmware/thead/thead,th1520-aon.h
index dae132b66873..d81f5f6f5b90 100644
--- a/include/linux/firmware/thead/thead,th1520-aon.h
+++ b/include/linux/firmware/thead/thead,th1520-aon.h
@@ -97,80 +97,6 @@ struct th1520_aon_rpc_ack_common {
 #define RPC_GET_SVC_FLAG_ACK_TYPE(MESG) (((MESG)->svc & 0x40) >> 6)
 #define RPC_SET_SVC_FLAG_ACK_TYPE(MESG, ACK) ((MESG)->svc |= (ACK) << 6)
 
-#define RPC_SET_BE64(MESG, OFFSET, SET_DATA)                                \
-	do {                                                                \
-		u8 *data = (u8 *)(MESG);                                    \
-		u64 _offset = (OFFSET);                                     \
-		u64 _set_data = (SET_DATA);                                 \
-		data[_offset + 7] = _set_data & 0xFF;                       \
-		data[_offset + 6] = (_set_data & 0xFF00) >> 8;              \
-		data[_offset + 5] = (_set_data & 0xFF0000) >> 16;           \
-		data[_offset + 4] = (_set_data & 0xFF000000) >> 24;         \
-		data[_offset + 3] = (_set_data & 0xFF00000000) >> 32;       \
-		data[_offset + 2] = (_set_data & 0xFF0000000000) >> 40;     \
-		data[_offset + 1] = (_set_data & 0xFF000000000000) >> 48;   \
-		data[_offset + 0] = (_set_data & 0xFF00000000000000) >> 56; \
-	} while (0)
-
-#define RPC_SET_BE32(MESG, OFFSET, SET_DATA)			    \
-	do {							    \
-		u8 *data = (u8 *)(MESG);			    \
-		u64 _offset = (OFFSET);				    \
-		u64 _set_data = (SET_DATA);			    \
-		data[_offset + 3] = (_set_data) & 0xFF;		    \
-		data[_offset + 2] = (_set_data & 0xFF00) >> 8;	    \
-		data[_offset + 1] = (_set_data & 0xFF0000) >> 16;   \
-		data[_offset + 0] = (_set_data & 0xFF000000) >> 24; \
-	} while (0)
-
-#define RPC_SET_BE16(MESG, OFFSET, SET_DATA)		       \
-	do {						       \
-		u8 *data = (u8 *)(MESG);		       \
-		u64 _offset = (OFFSET);			       \
-		u64 _set_data = (SET_DATA);		       \
-		data[_offset + 1] = (_set_data) & 0xFF;	       \
-		data[_offset + 0] = (_set_data & 0xFF00) >> 8; \
-	} while (0)
-
-#define RPC_SET_U8(MESG, OFFSET, SET_DATA)	  \
-	do {					  \
-		u8 *data = (u8 *)(MESG);	  \
-		data[OFFSET] = (SET_DATA) & 0xFF; \
-	} while (0)
-
-#define RPC_GET_BE64(MESG, OFFSET, PTR)                                      \
-	do {                                                                 \
-		u8 *data = (u8 *)(MESG);                                     \
-		u64 _offset = (OFFSET);                                      \
-		*(u32 *)(PTR) =                                              \
-			(data[_offset + 7] | data[_offset + 6] << 8 |        \
-			 data[_offset + 5] << 16 | data[_offset + 4] << 24 | \
-			 data[_offset + 3] << 32 | data[_offset + 2] << 40 | \
-			 data[_offset + 1] << 48 | data[_offset + 0] << 56); \
-	} while (0)
-
-#define RPC_GET_BE32(MESG, OFFSET, PTR)                                      \
-	do {                                                                 \
-		u8 *data = (u8 *)(MESG);                                     \
-		u64 _offset = (OFFSET);                                      \
-		*(u32 *)(PTR) =                                              \
-			(data[_offset + 3] | data[_offset + 2] << 8 |        \
-			 data[_offset + 1] << 16 | data[_offset + 0] << 24); \
-	} while (0)
-
-#define RPC_GET_BE16(MESG, OFFSET, PTR)                                       \
-	do {                                                                  \
-		u8 *data = (u8 *)(MESG);                                      \
-		u64 _offset = (OFFSET);                                       \
-		*(u16 *)(PTR) = (data[_offset + 1] | data[_offset + 0] << 8); \
-	} while (0)
-
-#define RPC_GET_U8(MESG, OFFSET, PTR)          \
-	do {                                   \
-		u8 *data = (u8 *)(MESG);       \
-		*(u8 *)(PTR) = (data[OFFSET]); \
-	} while (0)
-
 /*
  * Defines for SC PM Power Mode
  */
-- 
2.34.1

Re: [PATCH v1] firmware: thead: Fix buffer overflow and use standard endian macros

[Dan Carpenter]

On Thu, Apr 03, 2025 at 03:10:51PM +0200, Michal Wilczynski wrote:
> Addresses two issues in the TH1520 AON firmware protocol driver:
> 
> 1. Fix a potential buffer overflow where the code used unsafe pointer
>    arithmetic to access the 'mode' field through the 'resource' pointer
>    with an offset. This was flagged by Smatch static checker as:
>    "buffer overflow 'data' 2 <= 3"
> 
> 2. Replace custom RPC_SET_BE* and RPC_GET_BE* macros with standard
>    kernel endianness conversion macros (cpu_to_be16, etc.) for better
>    portability and maintainability.
> 
> The functionality was re-tested with the GPU power-up sequence,
> confirming the GPU powers up correctly and the driver probes
> successfully.
> 
> [   12.702370] powervr ffef400000.gpu: [drm] loaded firmware
> powervr/rogue_36.52.104.182_v1.fw
> [   12.711043] powervr ffef400000.gpu: [drm] FW version v1.0 (build
> 6645434 OS)
> [   12.719787] [drm] Initialized powervr 1.0.0 for ffef400000.gpu on
> minor 0
> 
> Fixes: e4b3cbd840e5 ("firmware: thead: Add AON firmware protocol driver")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/17a0ccce-060b-4b9d-a3c4-8d5d5823b1c9@stanley.mountain/
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---

This is nice.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

Re: [PATCH v1] firmware: thead: Fix buffer overflow and use standard endian macros

[Drew Fustini]

On Thu, Apr 03, 2025 at 03:10:51PM +0200, Michal Wilczynski wrote:
> Addresses two issues in the TH1520 AON firmware protocol driver:
> 
> 1. Fix a potential buffer overflow where the code used unsafe pointer
>    arithmetic to access the 'mode' field through the 'resource' pointer
>    with an offset. This was flagged by Smatch static checker as:
>    "buffer overflow 'data' 2 <= 3"
> 
> 2. Replace custom RPC_SET_BE* and RPC_GET_BE* macros with standard
>    kernel endianness conversion macros (cpu_to_be16, etc.) for better
>    portability and maintainability.
> 
> The functionality was re-tested with the GPU power-up sequence,
> confirming the GPU powers up correctly and the driver probes
> successfully.
> 
> [   12.702370] powervr ffef400000.gpu: [drm] loaded firmware
> powervr/rogue_36.52.104.182_v1.fw
> [   12.711043] powervr ffef400000.gpu: [drm] FW version v1.0 (build
> 6645434 OS)
> [   12.719787] [drm] Initialized powervr 1.0.0 for ffef400000.gpu on
> minor 0
> 
> Fixes: e4b3cbd840e5 ("firmware: thead: Add AON firmware protocol driver")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/17a0ccce-060b-4b9d-a3c4-8d5d5823b1c9@stanley.mountain/
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>

Acked-by: Drew Fustini <fustini@kernel.org>

Is Ulf the person that would take this fix?

Thanks,
Drew

Re: [PATCH v1] firmware: thead: Fix buffer overflow and use standard endian macros

[Michal Wilczynski]

On 3/26/26 20:14, Drew Fustini wrote:
> On Thu, Apr 03, 2025 at 03:10:51PM +0200, Michal Wilczynski wrote:
>> Addresses two issues in the TH1520 AON firmware protocol driver:
>>
>> 1. Fix a potential buffer overflow where the code used unsafe pointer
>>    arithmetic to access the 'mode' field through the 'resource' pointer
>>    with an offset. This was flagged by Smatch static checker as:
>>    "buffer overflow 'data' 2 <= 3"
>>
>> 2. Replace custom RPC_SET_BE* and RPC_GET_BE* macros with standard
>>    kernel endianness conversion macros (cpu_to_be16, etc.) for better
>>    portability and maintainability.
>>
>> The functionality was re-tested with the GPU power-up sequence,
>> confirming the GPU powers up correctly and the driver probes
>> successfully.
>>
>> [   12.702370] powervr ffef400000.gpu: [drm] loaded firmware
>> powervr/rogue_36.52.104.182_v1.fw
>> [   12.711043] powervr ffef400000.gpu: [drm] FW version v1.0 (build
>> 6645434 OS)
>> [   12.719787] [drm] Initialized powervr 1.0.0 for ffef400000.gpu on
>> minor 0
>>
>> Fixes: e4b3cbd840e5 ("firmware: thead: Add AON firmware protocol driver")
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/all/17a0ccce-060b-4b9d-a3c4-8d5d5823b1c9@stanley.mountain/
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> 
> Acked-by: Drew Fustini <fustini@kernel.org>
> 
> Is Ulf the person that would take this fix?
> 
> Thanks,
> Drew
> 

Hi,
I think it would be great if Ulf could take the patch.

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>