[PATCH v1] driver: handle a NULL pointer reference on the error path

[Göktürk Yüksek]

In the function jailhouse_hypervisor_enable(), jumping to the label
error_unmap results in a call to jailhouse_free_firmware() which
releases hypervisor_mem_res and sets it to NULL. However, the
execution proceeds to the label error_release_memreg and tries to
access hypervisor_mem_res->start, which triggers a NULL pointer
reference.

Fix the problem for explicitly checking against the NULL pointer.

Signed-off-by: Göktürk Yüksek <gok...@binghamton.edu>
---
driver/main.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/driver/main.c b/driver/main.c
index 732a54a..8e0e79e 100644
--- a/driver/main.c
+++ b/driver/main.c
@@ -558,8 +558,11 @@ error_unmap:
iounmap(clock_reg);

error_release_memreg:
- release_mem_region(hypervisor_mem_res->start,
- resource_size(hypervisor_mem_res));
+ /* It is possible that a previous call to jailhouse_firmware_free()
+ * might have released the hypervisor_mem already. */
+ if (hypervisor_mem_res)
+ release_mem_region(hypervisor_mem_res->start,
+ resource_size(hypervisor_mem_res));
hypervisor_mem_res = NULL;

error_release_fw:
--
2.10.2

[Jan Kiszka]

On 2017-07-31 23:07, Göktürk Yüksek wrote:
> In the function jailhouse_hypervisor_enable(), jumping to the label
> error_unmap results in a call to jailhouse_free_firmware() which
> releases hypervisor_mem_res and sets it to NULL. However, the
> execution proceeds to the label error_release_memreg and tries to
> access hypervisor_mem_res->start, which triggers a NULL pointer
> reference.
>
> Fix the problem for explicitly checking against the NULL pointer.
>
> Signed-off-by: Göktürk Yüksek <gok...@binghamton.edu>

Good catch!

> ---
> driver/main.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/driver/main.c b/driver/main.c
> index 732a54a..8e0e79e 100644
> --- a/driver/main.c
> +++ b/driver/main.c
> @@ -558,8 +558,11 @@ error_unmap:
> iounmap(clock_reg);
>
> error_release_memreg:
> - release_mem_region(hypervisor_mem_res->start,
> - resource_size(hypervisor_mem_res));
> + /* It is possible that a previous call to jailhouse_firmware_free()
> + * might have released the hypervisor_mem already. */

Let me rephrase this less subjunctively: if jailhouse_firmware_free was
called before, it "... has released the hypervisor_mem_res already".

Will tune this on merge.

> + if (hypervisor_mem_res)
> + release_mem_region(hypervisor_mem_res->start,
> + resource_size(hypervisor_mem_res));
> hypervisor_mem_res = NULL;
>
> error_release_fw:
>

Thanks,
Jan

--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux