| Message ID | YxDQpc9IINUuUhQr@kili (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: cavium - prevent integer overflow loading firmware | expand |
On Thu, Sep 01, 2022 at 06:32:53PM +0300, Dan Carpenter wrote: > > @@ -263,7 +264,13 @@ 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; > + > + code_length = ntohl(ucode->code_length); > + if (code_length >= INT_MAX / 2) { > + ret = -EINVAL; > + goto fw_release; > + } > + mcode->code_size = code_length; Where did the "* 2" go? BTW, what is the threat model here? If the firmware metadata can't be trusted, shouldn't we be capping the firmware size at a level a lot lower than INT_MAX? Cheers,
On Thu, Sep 08, 2022 at 05:42:54PM +0800, Herbert Xu wrote: > On Thu, Sep 01, 2022 at 06:32:53PM +0300, Dan Carpenter wrote: > > > > @@ -263,7 +264,13 @@ 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; > > + > > + code_length = ntohl(ucode->code_length); > > + if (code_length >= INT_MAX / 2) { > > + ret = -EINVAL; > > + goto fw_release; > > + } > > + mcode->code_size = code_length; > > Where did the "* 2" go? Crud. :/ Sorry. > > BTW, what is the threat model here? If the firmware metadata can't > be trusted, shouldn't we be capping the firmware size at a level > a lot lower than INT_MAX? This is not firmware metadata, I'm fairly sure the fw_entry->data is raw data from the file system. Realistically, if you can't trust your firmware then you are probably toasted but there is a move to trust as little as possible. Also Smatch marks data from the file system as untrusted so it will generate static checker warnings. regards, dan carpenter
diff --git a/drivers/crypto/cavium/cpt/cptpf_main.c b/drivers/crypto/cavium/cpt/cptpf_main.c index 8c32d0eb8fcf..b196579dcd98 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,7 +264,13 @@ 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; + + code_length = ntohl(ucode->code_length); + if (code_length >= INT_MAX / 2) { + ret = -EINVAL; + goto fw_release; + } + mcode->code_size = code_length; if (!mcode->code_size) { ret = -EINVAL; goto fw_release;
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> --- drivers/crypto/cavium/cpt/cptpf_main.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)