| Message ID | ZK0Q5nbLyDO7kJa+@moroto (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | accel/qaic: Improve bounds checking in encode/decode | expand |
On 7/11/2023 1:50 PM, Dan Carpenter wrote: > Copy the bounds checking from encode_message() to decode_message(). > > This patch addresses the following concerns. Ensure that there is > enough space for at least one header so that we don't have a negative > size later. > > if (msg_hdr_len < sizeof(*trans_hdr)) > > Ensure that we have enough space to read the next header from the > msg->data. > > if (msg_len > msg_hdr_len - sizeof(*trans_hdr)) > return -EINVAL; > > Check that the trans_hdr->len is not below the minimum size: > > if (hdr_len < sizeof(*trans_hdr)) > > This minimum check ensures that we don't corrupt memory in > decode_passthrough() when we do. > > memcpy(out_trans->data, in_trans->data, len - sizeof(in_trans->hdr)); > > And finally, use size_add() to prevent an integer overflow: > > if (size_add(msg_len, hdr_len) > msg_hdr_len) > > Fixes: 129776ac2e38 ("accel/qaic: Add control path") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
On 7/14/2023 5:42 AM, Pranjal Ramajor Asha Kanojiya wrote: > > > On 7/11/2023 1:50 PM, Dan Carpenter wrote: >> Copy the bounds checking from encode_message() to decode_message(). >> >> This patch addresses the following concerns. Ensure that there is >> enough space for at least one header so that we don't have a negative >> size later. >> >> if (msg_hdr_len < sizeof(*trans_hdr)) >> >> Ensure that we have enough space to read the next header from the >> msg->data. >> >> if (msg_len > msg_hdr_len - sizeof(*trans_hdr)) >> return -EINVAL; >> >> Check that the trans_hdr->len is not below the minimum size: >> >> if (hdr_len < sizeof(*trans_hdr)) >> >> This minimum check ensures that we don't corrupt memory in >> decode_passthrough() when we do. >> >> memcpy(out_trans->data, in_trans->data, len - sizeof(in_trans->hdr)); >> >> And finally, use size_add() to prevent an integer overflow: >> >> if (size_add(msg_len, hdr_len) > msg_hdr_len) >> >> Fixes: 129776ac2e38 ("accel/qaic: Add control path") >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > 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 2fdd5959c52f..752b67aff777 100644 --- a/drivers/accel/qaic/qaic_control.c +++ b/drivers/accel/qaic/qaic_control.c @@ -956,15 +956,23 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg, int ret; int i; - if (msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH) + if (msg_hdr_len < sizeof(*trans_hdr) || + msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH) return -EINVAL; user_msg->len = 0; user_msg->count = le32_to_cpu(msg->hdr.count); for (i = 0; i < user_msg->count; ++i) { + u32 hdr_len; + + if (msg_len > msg_hdr_len - sizeof(*trans_hdr)) + return -EINVAL; + trans_hdr = (struct wire_trans_hdr *)(msg->data + msg_len); - if (msg_len + le32_to_cpu(trans_hdr->len) > msg_hdr_len) + hdr_len = le32_to_cpu(trans_hdr->len); + if (hdr_len < sizeof(*trans_hdr) || + size_add(msg_len, hdr_len) > msg_hdr_len) return -EINVAL; switch (le32_to_cpu(trans_hdr->type)) {
Copy the bounds checking from encode_message() to decode_message(). This patch addresses the following concerns. Ensure that there is enough space for at least one header so that we don't have a negative size later. if (msg_hdr_len < sizeof(*trans_hdr)) Ensure that we have enough space to read the next header from the msg->data. if (msg_len > msg_hdr_len - sizeof(*trans_hdr)) return -EINVAL; Check that the trans_hdr->len is not below the minimum size: if (hdr_len < sizeof(*trans_hdr)) This minimum check ensures that we don't corrupt memory in decode_passthrough() when we do. memcpy(out_trans->data, in_trans->data, len - sizeof(in_trans->hdr)); And finally, use size_add() to prevent an integer overflow: if (size_add(msg_len, hdr_len) > msg_hdr_len) Fixes: 129776ac2e38 ("accel/qaic: Add control path") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: Fix the >= vs > bug in "if (msg_len > msg_hdr_len - sizeof(*trans_hdr))" --- drivers/accel/qaic/qaic_control.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)