| Message ID | e94567c5-0478-4bdf-84fc-5709df0b3392@moroto.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/qaic: Improve bounds checking in encode/decode | expand |
On 6/21/2023 12:51 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> > --- > drivers/accel/qaic/qaic_control.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > index a51b1594dcfa..78f6c3d6380d 100644 > --- a/drivers/accel/qaic/qaic_control.c > +++ b/drivers/accel/qaic/qaic_control.c > @@ -955,15 +955,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; If I understand correctly this check is added to verify if we are left with trans_hdr size of data. In that case '>' comparison operator should be used. > + > 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)) {
On 6/21/2023 1:21 AM, 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> > --- > drivers/accel/qaic/qaic_control.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c > index a51b1594dcfa..78f6c3d6380d 100644 > --- a/drivers/accel/qaic/qaic_control.c > +++ b/drivers/accel/qaic/qaic_control.c > @@ -955,15 +955,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) || How I view this - does the specified message length contain at-least one transaction for us to decode? Seems ok at first glance. However, the header length is not just the length of the payload, but also the header itself. So sizeof(*trans_hdr) should be added to sizeof(struct wire_msg_hdr). Otherwise msg_hdr_len could be 64 (just the size of wire_msg_hdr) which is more than sizeof(*trans_hdr) but still means we don't have a transaction to decode. > + 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)) {
diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c index a51b1594dcfa..78f6c3d6380d 100644 --- a/drivers/accel/qaic/qaic_control.c +++ b/drivers/accel/qaic/qaic_control.c @@ -955,15 +955,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> --- drivers/accel/qaic/qaic_control.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)