| Message ID | ZK0Q7IsPkj6WSCcL@moroto (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | accel/qaic: Improve bounds checking in encode/decode | expand |
On 7/11/2023 1:51 PM, Dan Carpenter wrote: > The encode_dma() function has integer overflow checks. The > encode_passthrough(), encode_activate() and encode_status() functions > did not. I added integer overflow checking everywhere. I also > updated the integer overflow checking in encode_dma() to use size_add() > so everything is consistent. > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: no change > > drivers/accel/qaic/qaic_control.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > index 752b67aff777..23680f5f1902 100644 > --- a/drivers/accel/qaic/qaic_control.c > +++ b/drivers/accel/qaic/qaic_control.c > @@ -367,7 +367,7 @@ static int encode_passthrough(struct qaic_device *qdev, void *trans, struct wrap > if (in_trans->hdr.len % 8 != 0) > return -EINVAL; > > - if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_EXT_MSG_LENGTH) > + if (size_add(msg_hdr_len, in_trans->hdr.len) > QAIC_MANAGE_EXT_MSG_LENGTH) > return -ENOSPC; > > trans_wrapper = add_wrapper(wrappers, > @@ -558,12 +558,10 @@ static int encode_dma(struct qaic_device *qdev, void *trans, struct wrapper_list > msg = &wrapper->msg; > msg_hdr_len = le32_to_cpu(msg->hdr.len); > > - if (msg_hdr_len > (UINT_MAX - QAIC_MANAGE_EXT_MSG_LENGTH)) > - return -EINVAL; > - > /* There should be enough space to hold at least one ASP entry. */ > - if (msg_hdr_len + sizeof(*out_trans) + sizeof(struct wire_addr_size_pair) > > - QAIC_MANAGE_EXT_MSG_LENGTH) > + if (size_add(msg_hdr_len, > + sizeof(*out_trans) + sizeof(struct wire_addr_size_pair)) > Can we have the entire size_add() on same line? > + QAIC_MANAGE_EXT_MSG_LENGTH) > return -ENOMEM; > > if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size) > @@ -635,7 +633,7 @@ static int encode_activate(struct qaic_device *qdev, void *trans, struct wrapper > msg = &wrapper->msg; > msg_hdr_len = le32_to_cpu(msg->hdr.len); > > - if (msg_hdr_len + sizeof(*out_trans) > QAIC_MANAGE_MAX_MSG_LENGTH) > + if (size_add(msg_hdr_len, sizeof(*out_trans)) > QAIC_MANAGE_MAX_MSG_LENGTH) > return -ENOSPC; > > if (!in_trans->queue_size) > @@ -719,7 +717,7 @@ static int encode_status(struct qaic_device *qdev, void *trans, struct wrapper_l > msg = &wrapper->msg; > msg_hdr_len = le32_to_cpu(msg->hdr.len); > > - if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_MAX_MSG_LENGTH) > + if (size_add(msg_hdr_len, in_trans->hdr.len) > QAIC_MANAGE_MAX_MSG_LENGTH) > return -ENOSPC; > > trans_wrapper = add_wrapper(wrappers, sizeof(*trans_wrapper)); Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
On 7/14/2023 5:44 AM, Pranjal Ramajor Asha Kanojiya wrote: > > > On 7/11/2023 1:51 PM, Dan Carpenter wrote: >> The encode_dma() function has integer overflow checks. The >> encode_passthrough(), encode_activate() and encode_status() functions >> did not. I added integer overflow checking everywhere. I also >> updated the integer overflow checking in encode_dma() to use size_add() >> so everything is consistent. >> >> Fixes: 129776ac2e38 ("accel/qaic: Add control path") >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> v2: no change >> >> drivers/accel/qaic/qaic_control.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/accel/qaic/qaic_control.c >> b/drivers/accel/qaic/qaic_control.c >> index 752b67aff777..23680f5f1902 100644 >> --- a/drivers/accel/qaic/qaic_control.c >> +++ b/drivers/accel/qaic/qaic_control.c >> @@ -367,7 +367,7 @@ static int encode_passthrough(struct qaic_device >> *qdev, void *trans, struct wrap >> if (in_trans->hdr.len % 8 != 0) >> return -EINVAL; >> - if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_EXT_MSG_LENGTH) >> + if (size_add(msg_hdr_len, in_trans->hdr.len) > >> QAIC_MANAGE_EXT_MSG_LENGTH) >> return -ENOSPC; >> trans_wrapper = add_wrapper(wrappers, >> @@ -558,12 +558,10 @@ static int encode_dma(struct qaic_device *qdev, >> void *trans, struct wrapper_list >> msg = &wrapper->msg; >> msg_hdr_len = le32_to_cpu(msg->hdr.len); >> - if (msg_hdr_len > (UINT_MAX - QAIC_MANAGE_EXT_MSG_LENGTH)) >> - return -EINVAL; >> - >> /* There should be enough space to hold at least one ASP entry. */ >> - if (msg_hdr_len + sizeof(*out_trans) + sizeof(struct >> wire_addr_size_pair) > >> - QAIC_MANAGE_EXT_MSG_LENGTH) >> + if (size_add(msg_hdr_len, >> + sizeof(*out_trans) + sizeof(struct wire_addr_size_pair)) > > > Can we have the entire size_add() on same line? > >> + QAIC_MANAGE_EXT_MSG_LENGTH) >> return -ENOMEM; >> if (in_trans->addr + in_trans->size < in_trans->addr || >> !in_trans->size) >> @@ -635,7 +633,7 @@ static int encode_activate(struct qaic_device >> *qdev, void *trans, struct wrapper >> msg = &wrapper->msg; >> msg_hdr_len = le32_to_cpu(msg->hdr.len); >> - if (msg_hdr_len + sizeof(*out_trans) > QAIC_MANAGE_MAX_MSG_LENGTH) >> + if (size_add(msg_hdr_len, sizeof(*out_trans)) > >> QAIC_MANAGE_MAX_MSG_LENGTH) >> return -ENOSPC; >> if (!in_trans->queue_size) >> @@ -719,7 +717,7 @@ static int encode_status(struct qaic_device *qdev, >> void *trans, struct wrapper_l >> msg = &wrapper->msg; >> msg_hdr_len = le32_to_cpu(msg->hdr.len); >> - if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_MAX_MSG_LENGTH) >> + if (size_add(msg_hdr_len, in_trans->hdr.len) > >> QAIC_MANAGE_MAX_MSG_LENGTH) >> return -ENOSPC; >> trans_wrapper = add_wrapper(wrappers, sizeof(*trans_wrapper)); > > Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com> Pushed to drm-misc-fixes -Jeff
diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c index 752b67aff777..23680f5f1902 100644 --- a/drivers/accel/qaic/qaic_control.c +++ b/drivers/accel/qaic/qaic_control.c @@ -367,7 +367,7 @@ static int encode_passthrough(struct qaic_device *qdev, void *trans, struct wrap if (in_trans->hdr.len % 8 != 0) return -EINVAL; - if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_EXT_MSG_LENGTH) + if (size_add(msg_hdr_len, in_trans->hdr.len) > QAIC_MANAGE_EXT_MSG_LENGTH) return -ENOSPC; trans_wrapper = add_wrapper(wrappers, @@ -558,12 +558,10 @@ static int encode_dma(struct qaic_device *qdev, void *trans, struct wrapper_list msg = &wrapper->msg; msg_hdr_len = le32_to_cpu(msg->hdr.len); - if (msg_hdr_len > (UINT_MAX - QAIC_MANAGE_EXT_MSG_LENGTH)) - return -EINVAL; - /* There should be enough space to hold at least one ASP entry. */ - if (msg_hdr_len + sizeof(*out_trans) + sizeof(struct wire_addr_size_pair) > - QAIC_MANAGE_EXT_MSG_LENGTH) + if (size_add(msg_hdr_len, + sizeof(*out_trans) + sizeof(struct wire_addr_size_pair)) > + QAIC_MANAGE_EXT_MSG_LENGTH) return -ENOMEM; if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size) @@ -635,7 +633,7 @@ static int encode_activate(struct qaic_device *qdev, void *trans, struct wrapper msg = &wrapper->msg; msg_hdr_len = le32_to_cpu(msg->hdr.len); - if (msg_hdr_len + sizeof(*out_trans) > QAIC_MANAGE_MAX_MSG_LENGTH) + if (size_add(msg_hdr_len, sizeof(*out_trans)) > QAIC_MANAGE_MAX_MSG_LENGTH) return -ENOSPC; if (!in_trans->queue_size) @@ -719,7 +717,7 @@ static int encode_status(struct qaic_device *qdev, void *trans, struct wrapper_l msg = &wrapper->msg; msg_hdr_len = le32_to_cpu(msg->hdr.len); - if (msg_hdr_len + in_trans->hdr.len > QAIC_MANAGE_MAX_MSG_LENGTH) + if (size_add(msg_hdr_len, in_trans->hdr.len) > QAIC_MANAGE_MAX_MSG_LENGTH) return -ENOSPC; trans_wrapper = add_wrapper(wrappers, sizeof(*trans_wrapper));
The encode_dma() function has integer overflow checks. The encode_passthrough(), encode_activate() and encode_status() functions did not. I added integer overflow checking everywhere. I also updated the integer overflow checking in encode_dma() to use size_add() so everything is consistent. Fixes: 129776ac2e38 ("accel/qaic: Add control path") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: no change drivers/accel/qaic/qaic_control.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)