[PATCH 1/2] firmware: zynqmp: Handle errors from ipi_req properly

There are multiple errors what can happen in ipi_req but they are not propagated properly. That's why propage all error properly.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
drivers/firmware/firmware-zynqmp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index c22bdca282fc..1391aab0a160 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -165,6 +165,7 @@ int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2, * firmware API is limited by the SMC call size */ u32 regs[] = {api_id, arg0, arg1, arg2, arg3}; + int ret;
if (api_id == PM_FPGA_LOAD) { /* Swap addr_hi/low because of incompatibility */ @@ -174,7 +175,10 @@ int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2, regs[2] = temp; }
- ipi_req(regs, PAYLOAD_ARG_CNT, ret_payload, PAYLOAD_ARG_CNT); + ret = ipi_req(regs, PAYLOAD_ARG_CNT, ret_payload, + PAYLOAD_ARG_CNT); + if (ret) + return ret; #else return -EPERM; #endif

When a caller is not interested in the returned message, the ret_payload pointer is set to NULL in the u-boot-sources. In this case, under EL3, the memory from address 0x0 would be overwritten by ipi_req() with the returned IPI message, damaging the original data under this address. The patch, in case ret_payload is NULL, assigns the pointer to the array holding the IPI message being sent.
Signed-off-by: Adrian Fiergolski adrian.fiergolski@fastree3d.com Signed-off-by: Michal Simek michal.simek@xilinx.com ---
Based on origin series from Adrian. That's why also adding his SoB line. https://lore.kernel.org/r/20211014124349.1429696-1-adrian.fiergolski@fastree...
Adrian: The patch is just suggestion how we could avoid that NULL pointer writes but done in ipi_req()
--- drivers/firmware/firmware-zynqmp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 1391aab0a160..7e498d8169e8 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -30,6 +30,10 @@ static int ipi_req(const u32 *req, size_t req_len, u32 *res, size_t res_maxlen) { struct zynqmp_ipi_msg msg; int ret; + u32 buffer[PAYLOAD_ARG_CNT]; + + if (!res) + res = buffer;
if (req_len > PMUFW_PAYLOAD_ARG_CNT || res_maxlen > PMUFW_PAYLOAD_ARG_CNT)

On 15.10.2021 16:57, Michal Simek wrote:
When a caller is not interested in the returned message, the ret_payload pointer is set to NULL in the u-boot-sources. In this case, under EL3, the memory from address 0x0 would be overwritten by ipi_req() with the returned IPI message, damaging the original data under this address. The patch, in case ret_payload is NULL, assigns the pointer to the array holding the IPI message being sent.
Signed-off-by: Adrian Fiergolski adrian.fiergolski@fastree3d.com Signed-off-by: Michal Simek michal.simek@xilinx.com
Reviewed-by: Adrian Fiergolski Adrian.Fiergolski@fastree3d.com
Thanks,
Adrian
Based on origin series from Adrian. That's why also adding his SoB line. https://lore.kernel.org/r/20211014124349.1429696-1-adrian.fiergolski@fastree...
Adrian: The patch is just suggestion how we could avoid that NULL pointer writes but done in ipi_req()
drivers/firmware/firmware-zynqmp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 1391aab0a160..7e498d8169e8 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -30,6 +30,10 @@ static int ipi_req(const u32 *req, size_t req_len, u32 *res, size_t res_maxlen) { struct zynqmp_ipi_msg msg; int ret;
u32 buffer[PAYLOAD_ARG_CNT];
if (!res)
res = buffer;
if (req_len > PMUFW_PAYLOAD_ARG_CNT || res_maxlen > PMUFW_PAYLOAD_ARG_CNT)

On 10/15/21 16:57, Michal Simek wrote:
When a caller is not interested in the returned message, the ret_payload pointer is set to NULL in the u-boot-sources. In this case, under EL3, the memory from address 0x0 would be overwritten by ipi_req() with the returned IPI message, damaging the original data under this address. The patch, in case ret_payload is NULL, assigns the pointer to the array holding the IPI message being sent.
Signed-off-by: Adrian Fiergolski adrian.fiergolski@fastree3d.com Signed-off-by: Michal Simek michal.simek@xilinx.com
Based on origin series from Adrian. That's why also adding his SoB line. https://lore.kernel.org/r/20211014124349.1429696-1-adrian.fiergolski@fastree...
Adrian: The patch is just suggestion how we could avoid that NULL pointer writes but done in ipi_req()
drivers/firmware/firmware-zynqmp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 1391aab0a160..7e498d8169e8 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -30,6 +30,10 @@ static int ipi_req(const u32 *req, size_t req_len, u32 *res, size_t res_maxlen) { struct zynqmp_ipi_msg msg; int ret;
u32 buffer[PAYLOAD_ARG_CNT];
if (!res)
res = buffer;
if (req_len > PMUFW_PAYLOAD_ARG_CNT || res_maxlen > PMUFW_PAYLOAD_ARG_CNT)
Applied. M

On 15.10.2021 16:57, Michal Simek wrote:
There are multiple errors what can happen in ipi_req but they are not propagated properly. That's why propage all error properly.
Signed-off-by: Michal Simek michal.simek@xilinx.com
Reviewed-by: Adrian Fiergolski Adrian.Fiergolski@fastree3d.com
Thanks,
Adrian
drivers/firmware/firmware-zynqmp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index c22bdca282fc..1391aab0a160 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -165,6 +165,7 @@ int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2, * firmware API is limited by the SMC call size */ u32 regs[] = {api_id, arg0, arg1, arg2, arg3};
int ret;
if (api_id == PM_FPGA_LOAD) { /* Swap addr_hi/low because of incompatibility */
@@ -174,7 +175,10 @@ int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2, regs[2] = temp; }
ipi_req(regs, PAYLOAD_ARG_CNT, ret_payload, PAYLOAD_ARG_CNT);
ret = ipi_req(regs, PAYLOAD_ARG_CNT, ret_payload,
PAYLOAD_ARG_CNT);
if (ret)
return ret;
#else return -EPERM; #endif

On 10/15/21 16:57, Michal Simek wrote:
There are multiple errors what can happen in ipi_req but they are not propagated properly. That's why propage all error properly.
Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/firmware/firmware-zynqmp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index c22bdca282fc..1391aab0a160 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -165,6 +165,7 @@ int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2, * firmware API is limited by the SMC call size */ u32 regs[] = {api_id, arg0, arg1, arg2, arg3};
int ret;
if (api_id == PM_FPGA_LOAD) { /* Swap addr_hi/low because of incompatibility */
@@ -174,7 +175,10 @@ int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2, regs[2] = temp; }
ipi_req(regs, PAYLOAD_ARG_CNT, ret_payload, PAYLOAD_ARG_CNT);
ret = ipi_req(regs, PAYLOAD_ARG_CNT, ret_payload,
PAYLOAD_ARG_CNT);
if (ret)
#else return -EPERM; #endifreturn ret;
Applied. M
participants (3)
-
Adrian Fiergolski
-
Michal Simek
-
Michal Simek