cmd: efidebug: always check return code from get_variable()

Message ID 20210122014248.20514-1-takahiro.akashi@linaro.org
State Accepted
Commit 30f8222bb0016eef29afdd997bd1b3cdc486fdf9
Headers show
Series
  • cmd: efidebug: always check return code from get_variable()
Related show

Commit Message

AKASHI Takahiro Jan. 22, 2021, 1:42 a.m.
CID 316364 says:
> Null pointer dereferences  (FORWARD_NULL)

>	printf("Result total size: 0x%x\n", result->variable_total_size);

at do_efi_capsule_res().

The code is basically safe because a buffer for "result" is allocated
by malloc() and filled up by the second get_variable(), which fails any way
if the allocation has failed.

But the first (and second) get_variable() possibly returns an error other
than EFI_SUCCESS. We always need to check the return code from
get_variable() before accessing the data in "result".

While this change won't suppress CID 316364, the resulting code is much
safer.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

---
 cmd/efidebug.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.28.0

Comments

Heinrich Schuchardt Jan. 22, 2021, 5:54 a.m. | #1
On 1/22/21 2:42 AM, AKASHI Takahiro wrote:
> CID 316364 says:

>> Null pointer dereferences  (FORWARD_NULL)

>> 	printf("Result total size: 0x%x\n", result->variable_total_size);

> at do_efi_capsule_res().

>

> The code is basically safe because a buffer for "result" is allocated

> by malloc() and filled up by the second get_variable(), which fails any way

> if the allocation has failed.

>

> But the first (and second) get_variable() possibly returns an error other

> than EFI_SUCCESS. We always need to check the return code from

> get_variable() before accessing the data in "result".

>

> While this change won't suppress CID 316364, the resulting code is much

> safer.

>

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>


Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>


> ---

>   cmd/efidebug.c | 12 +++++++-----

>   1 file changed, 7 insertions(+), 5 deletions(-)

>

> diff --git a/cmd/efidebug.c b/cmd/efidebug.c

> index 9a2d4ddd5ef4..83bc2196a5a9 100644

> --- a/cmd/efidebug.c

> +++ b/cmd/efidebug.c

> @@ -189,14 +189,16 @@ static int do_efi_capsule_res(struct cmd_tbl *cmdtp, int flag,

>   	ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size, NULL));

>   	if (ret == EFI_BUFFER_TOO_SMALL) {

>   		result = malloc(size);

> +		if (!result)

> +			return CMD_RET_FAILURE;

>   		ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size,

>   						result));

> -		if (ret != EFI_SUCCESS) {

> -			free(result);

> -			printf("Failed to get %ls\n", var_name16);

> +	}

> +	if (ret != EFI_SUCCESS) {

> +		free(result);

> +		printf("Failed to get %ls\n", var_name16);

>

> -			return CMD_RET_FAILURE;

> -		}

> +		return CMD_RET_FAILURE;

>   	}

>

>   	printf("Result total size: 0x%x\n", result->variable_total_size);

>

Patch

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 9a2d4ddd5ef4..83bc2196a5a9 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -189,14 +189,16 @@  static int do_efi_capsule_res(struct cmd_tbl *cmdtp, int flag,
 	ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size, NULL));
 	if (ret == EFI_BUFFER_TOO_SMALL) {
 		result = malloc(size);
+		if (!result)
+			return CMD_RET_FAILURE;
 		ret = EFI_CALL(RT->get_variable(var_name16, &guid, NULL, &size,
 						result));
-		if (ret != EFI_SUCCESS) {
-			free(result);
-			printf("Failed to get %ls\n", var_name16);
+	}
+	if (ret != EFI_SUCCESS) {
+		free(result);
+		printf("Failed to get %ls\n", var_name16);
 
-			return CMD_RET_FAILURE;
-		}
+		return CMD_RET_FAILURE;
 	}
 
 	printf("Result total size: 0x%x\n", result->variable_total_size);