| Message ID | YxDQeeqY6u5EBn5H@kili (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: marvell/octeontx - prevent integer overflows | expand |
On Thu, Sep 01, 2022 at 06:32:09PM +0300, Dan Carpenter wrote: > > @@ -303,7 +304,13 @@ static int process_tar_file(struct device *dev, > if (get_ucode_type(ucode_hdr, &ucode_type)) > return 0; > > - ucode_size = ntohl(ucode_hdr->code_length) * 2; > + code_length = ntohl(ucode_hdr->code_length); > + if (code_length >= INT_MAX / 2) { > + dev_err(dev, "Invalid code_length %u\n", code_length); > + return -EINVAL; > + } > + > + ucode_size = code_length * 2; > if (!ucode_size || (size < round_up(ucode_size, 16) + > sizeof(struct otx_cpt_ucode_hdr) + OTX_CPT_UCODE_SIGN_LEN)) { > dev_err(dev, "Ucode %s invalid size\n", filename); How come you didn't add a "ucode_size > size" check like you did below? > @@ -896,9 +904,16 @@ static int ucode_load(struct device *dev, struct otx_cpt_ucode *ucode, > ucode_hdr = (struct otx_cpt_ucode_hdr *) fw->data; > memcpy(ucode->ver_str, ucode_hdr->ver_str, OTX_CPT_UCODE_VER_STR_SZ); > ucode->ver_num = ucode_hdr->ver_num; > - ucode->size = ntohl(ucode_hdr->code_length) * 2; > - if (!ucode->size || (fw->size < round_up(ucode->size, 16) > - + sizeof(struct otx_cpt_ucode_hdr) + OTX_CPT_UCODE_SIGN_LEN)) { > + code_length = ntohl(ucode_hdr->code_length); > + if (code_length >= INT_MAX / 2) { > + ret = -EINVAL; > + goto release_fw; > + } > + ucode->size = code_length * 2; > + if (!ucode->size || > + ucode->size > fw->size || > + (fw->size < round_up(ucode->size, 16) + > + sizeof(struct otx_cpt_ucode_hdr) + OTX_CPT_UCODE_SIGN_LEN)) { > dev_err(dev, "Ucode %s invalid size\n", ucode_filename); > ret = -EINVAL; > goto release_fw; > -- > 2.35.1 Thanks,
On Thu, Sep 08, 2022 at 05:34:42PM +0800, Herbert Xu wrote: > On Thu, Sep 01, 2022 at 06:32:09PM +0300, Dan Carpenter wrote: > > > > @@ -303,7 +304,13 @@ static int process_tar_file(struct device *dev, > > if (get_ucode_type(ucode_hdr, &ucode_type)) > > return 0; > > > > - ucode_size = ntohl(ucode_hdr->code_length) * 2; > > + code_length = ntohl(ucode_hdr->code_length); > > + if (code_length >= INT_MAX / 2) { > > + dev_err(dev, "Invalid code_length %u\n", code_length); > > + return -EINVAL; > > + } > > + > > + ucode_size = code_length * 2; > > if (!ucode_size || (size < round_up(ucode_size, 16) + > > sizeof(struct otx_cpt_ucode_hdr) + OTX_CPT_UCODE_SIGN_LEN)) { > > dev_err(dev, "Ucode %s invalid size\n", filename); > > How come you didn't add a "ucode_size > size" check like you did > below? > I'm really sorry. This was not my best work at all. The ucode_size was a mistake. It should have just been the check against INT_MAX. regards, dan carpenter
diff --git a/drivers/crypto/marvell/octeontx/otx_cptpf_ucode.c b/drivers/crypto/marvell/octeontx/otx_cptpf_ucode.c index 23c6edc70914..1be85820d677 100644 --- a/drivers/crypto/marvell/octeontx/otx_cptpf_ucode.c +++ b/drivers/crypto/marvell/octeontx/otx_cptpf_ucode.c @@ -286,6 +286,7 @@ static int process_tar_file(struct device *dev, struct tar_ucode_info_t *tar_info; struct otx_cpt_ucode_hdr *ucode_hdr; int ucode_type, ucode_size; + unsigned int code_length; /* * If size is less than microcode header size then don't report @@ -303,7 +304,13 @@ static int process_tar_file(struct device *dev, if (get_ucode_type(ucode_hdr, &ucode_type)) return 0; - ucode_size = ntohl(ucode_hdr->code_length) * 2; + code_length = ntohl(ucode_hdr->code_length); + if (code_length >= INT_MAX / 2) { + dev_err(dev, "Invalid code_length %u\n", code_length); + return -EINVAL; + } + + ucode_size = code_length * 2; if (!ucode_size || (size < round_up(ucode_size, 16) + sizeof(struct otx_cpt_ucode_hdr) + OTX_CPT_UCODE_SIGN_LEN)) { dev_err(dev, "Ucode %s invalid size\n", filename); @@ -886,6 +893,7 @@ static int ucode_load(struct device *dev, struct otx_cpt_ucode *ucode, { struct otx_cpt_ucode_hdr *ucode_hdr; const struct firmware *fw; + unsigned int code_length; int ret; set_ucode_filename(ucode, ucode_filename); @@ -896,9 +904,16 @@ static int ucode_load(struct device *dev, struct otx_cpt_ucode *ucode, ucode_hdr = (struct otx_cpt_ucode_hdr *) fw->data; memcpy(ucode->ver_str, ucode_hdr->ver_str, OTX_CPT_UCODE_VER_STR_SZ); ucode->ver_num = ucode_hdr->ver_num; - ucode->size = ntohl(ucode_hdr->code_length) * 2; - if (!ucode->size || (fw->size < round_up(ucode->size, 16) - + sizeof(struct otx_cpt_ucode_hdr) + OTX_CPT_UCODE_SIGN_LEN)) { + code_length = ntohl(ucode_hdr->code_length); + if (code_length >= INT_MAX / 2) { + ret = -EINVAL; + goto release_fw; + } + ucode->size = code_length * 2; + if (!ucode->size || + ucode->size > fw->size || + (fw->size < round_up(ucode->size, 16) + + sizeof(struct otx_cpt_ucode_hdr) + OTX_CPT_UCODE_SIGN_LEN)) { dev_err(dev, "Ucode %s invalid size\n", ucode_filename); ret = -EINVAL; goto release_fw;
The "code_length * 2" can overflow. The round_up(ucode_size, 16) + sizeof() expression can overflow too. Prevent these overflows. Fixes: d9110b0b01ff ("crypto: marvell - add support for OCTEON TX CPT engine") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- I chose INT_MAX because it's way higher than what kmalloc() can allocate and it makes the code simpler. I think there is a static checker which tells people to change these to UINT_MAX. Don't do that, or at least CC me if you do. .../crypto/marvell/octeontx/otx_cptpf_ucode.c | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)