efi_loader: Trim output buffer size correctly for tee variables

Message ID 20200721225037.450919-1-ilias.apalodimas@linaro.org
State Accepted
Commit db94dfbd525943b1bf4ecda81477cedfe70fc50e
Headers show
Series
  • efi_loader: Trim output buffer size correctly for tee variables
Related show

Commit Message

Ilias Apalodimas July 21, 2020, 10:50 p.m.
The current code does not trim the output buffer correctly.
In fact it doesn't trim the buffer at all, since it calculates a wrong
value for it, which isn't even applied.
So let's remove the unused temporary size variable and trim the buffer
correctly.
Since we are editing efi_get_next_variable_name_int(), fix an indentation
error along the way.

Fixes: f042e47e8fb43 ("efi_loader: Implement EFI variable handling via OP-TEE")
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

---
 lib/efi_loader/efi_variable_tee.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

-- 
2.28.0.rc1

Comments

Heinrich Schuchardt July 22, 2020, 6:02 p.m. | #1
On 22.07.20 00:50, Ilias Apalodimas wrote:
> The current code does not trim the output buffer correctly.

> In fact it doesn't trim the buffer at all, since it calculates a wrong

> value for it, which isn't even applied.

> So let's remove the unused temporary size variable and trim the buffer

> correctly.

> Since we are editing efi_get_next_variable_name_int(), fix an indentation

> error along the way.

>

> Fixes: f042e47e8fb43 ("efi_loader: Implement EFI variable handling via OP-TEE")

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> ---

>  lib/efi_loader/efi_variable_tee.c | 13 +++----------

>  1 file changed, 3 insertions(+), 10 deletions(-)

>

> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c

> index c0423489388a..0e5b4479d936 100644

> --- a/lib/efi_loader/efi_variable_tee.c

> +++ b/lib/efi_loader/efi_variable_tee.c

> @@ -410,7 +410,6 @@ efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,

>  	efi_uintn_t payload_size;

>  	efi_uintn_t out_name_size;

>  	efi_uintn_t in_name_size;

> -	efi_uintn_t tmp_dsize;

>  	u8 *comm_buf = NULL;

>  	efi_status_t ret;

>

> @@ -433,13 +432,8 @@ efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,

>  	}

>

>  	/* Trim output buffer size */

> -	tmp_dsize = *variable_name_size;

> -	if (in_name_size + tmp_dsize >

> -			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {

> -		tmp_dsize = max_payload_size -

> -				MM_VARIABLE_GET_NEXT_HEADER_SIZE -

> -				in_name_size;

> -	}

> +	if (out_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)

> +		out_name_size = max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;

>

>  	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;

>  	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,

> @@ -465,8 +459,7 @@ efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,

>  		goto out;

>

>  	guidcpy(guid, &var_getnext->guid);

> -	memcpy(variable_name, (u8 *)var_getnext->name,

> -	       var_getnext->name_size);

> +	memcpy(variable_name, (u8 *)var_getnext->name, var_getnext->name_size);


var_getnext->name is already a pointer. There is no need to convert this
to (u8 *) as memcpy expects (void *) and will accept any pointer.

I will remove that conversion when merging.

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


>

>  out:

>  	free(comm_buf);

>

Patch

diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index c0423489388a..0e5b4479d936 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -410,7 +410,6 @@  efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
 	efi_uintn_t payload_size;
 	efi_uintn_t out_name_size;
 	efi_uintn_t in_name_size;
-	efi_uintn_t tmp_dsize;
 	u8 *comm_buf = NULL;
 	efi_status_t ret;
 
@@ -433,13 +432,8 @@  efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
 	}
 
 	/* Trim output buffer size */
-	tmp_dsize = *variable_name_size;
-	if (in_name_size + tmp_dsize >
-			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
-		tmp_dsize = max_payload_size -
-				MM_VARIABLE_GET_NEXT_HEADER_SIZE -
-				in_name_size;
-	}
+	if (out_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
+		out_name_size = max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;
 
 	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
 	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
@@ -465,8 +459,7 @@  efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
 		goto out;
 
 	guidcpy(guid, &var_getnext->guid);
-	memcpy(variable_name, (u8 *)var_getnext->name,
-	       var_getnext->name_size);
+	memcpy(variable_name, (u8 *)var_getnext->name, var_getnext->name_size);
 
 out:
 	free(comm_buf);