[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-crypto-vger
Subject:    Re: [PATCH v2] crypto: cavium - prevent integer overflow loading firmware
From:       Christophe JAILLET <christophe.jaillet () wanadoo ! fr>
Date:       2022-09-30 16:32:53
Message-ID: cecca972-33c8-03a9-d632-c85ed06dff8b () wanadoo ! fr
[Download RAW message or body]

Le 19/09/2022 à 08:43, Dan Carpenter a écrit  :
> The "code_length" value comes from the firmware file.  If your firmware
> is untrusted realistically there is probably very little you can do to
> protect yourself.  Still we try to limit the damage as much as possible.
> Also Smatch marks any data read from the filesystem as untrusted and
> prints warnings if it not capped correctly.
> 
> The "ntohl(ucode->code_length) * 2" multiplication can have an
> integer overflow.
> 
> Fixes: 9e2c7d99941d ("crypto: cavium - Add Support for Octeon-tx CPT Engine")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The first code removed the " * 2" so it would have caused immediate
>      memory corruption and crashes.
> 
>      Also in version 2 I combine the "if (!mcode->code_size) {" check
>      with the overflow check for better readability.
> 
>   drivers/crypto/cavium/cpt/cptpf_main.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/cavium/cpt/cptpf_main.c b/drivers/crypto/cavium/cpt/cptpf_main.c
> index 8c32d0eb8fcf..6872ac344001 100644
> --- a/drivers/crypto/cavium/cpt/cptpf_main.c
> +++ b/drivers/crypto/cavium/cpt/cptpf_main.c
> @@ -253,6 +253,7 @@ static int cpt_ucode_load_fw(struct cpt_device *cpt, const u8 *fw, bool is_ae)
>   	const struct firmware *fw_entry;
>   	struct device *dev = &cpt->pdev->dev;
>   	struct ucode_header *ucode;
> +	unsigned int code_length;
>   	struct microcode *mcode;
>   	int j, ret = 0;
>   
> @@ -263,11 +264,12 @@ static int cpt_ucode_load_fw(struct cpt_device *cpt, const u8 *fw, bool is_ae)
>   	ucode = (struct ucode_header *)fw_entry->data;
>   	mcode = &cpt->mcode[cpt->next_mc_idx];
>   	memcpy(mcode->version, (u8 *)fw_entry->data, CPT_UCODE_VERSION_SZ);
> -	mcode->code_size = ntohl(ucode->code_length) * 2;
> -	if (!mcode->code_size) {
> +	code_length = ntohl(ucode->code_length);
> +	if (code_length == 0 || code_length >= INT_MAX / 2) {

Hi,

out of curiosity,

'code_length' is 'unsigned int'
'mcode->code_size' is u32.

Why not UINT_MAX / 2?

CJ

>   		ret = -EINVAL;
>   		goto fw_release;
>   	}
> +	mcode->code_size = code_length * 2;
>   
>   	mcode->is_ae = is_ae;
>   	mcode->core_mask = 0ULL;

[prev in list] [next in list] [prev in thread] [next in thread]