diff mbox series

[v2] crypto: cavium - prevent integer overflow loading firmware

Message ID YygPj8aYTvApOQFB@kili
State Accepted
Commit 2526d6bf27d15054bb0778b2f7bc6625fd934905
Headers show
Series [v2] crypto: cavium - prevent integer overflow loading firmware | expand

Commit Message

Dan Carpenter Sept. 19, 2022, 6:43 a.m. UTC 
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(-)

Comments

Herbert Xu Sept. 30, 2022, 6:14 a.m. UTC | #1 
On Mon, Sep 19, 2022 at 09:43:27AM +0300, Dan Carpenter wrote:
> 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(-)

Patch applied.  Thanks.
Christophe JAILLET Sept. 30, 2022, 4:32 p.m. UTC | #2 
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;
Dan Carpenter Oct. 13, 2022, 2:44 p.m. UTC | #3 
On Fri, Sep 30, 2022 at 06:32:53PM +0200, Christophe JAILLET wrote:
> 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?

Sorry for not responding earlier.  UINT_MAX / 2 would have worked here.

There was a similar issue in ucode_load() and in that code if you wanted
to use UINT_MAX then you would have had to write something like:

	if (code_length >= (UINT_MAX - 16) / 2)

That is sort of 9th grade algebra level of complicated.  But I've messed
it up basic algebra before and I've seen other people mess up their
integer overflow tests as well.

So I decided it was easier to just use INT_MAX / 2 consistently
everywhere.  The real values are not going to be anywhere near that
high so it doesn't affect runtime at all.

Also while I was writing this patch back in September, I saw someone
had changed one of INT_MAX checks to UINT_MAX.  For no reason at all.
It doesn't affect runtime.  They didn't tell me at all.  I was
unspeakably annoyed and I vowed to hold a grudge about this for all
time.  But unfortunately I forgotten the details so they're essentially
forgiven at this point...

regards,
dan carpenter
diff mbox series

Patch

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) {
 		ret = -EINVAL;
 		goto fw_release;
 	}
+	mcode->code_size = code_length * 2;
 
 	mcode->is_ae = is_ae;
 	mcode->core_mask = 0ULL;