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: 129776ac2e ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Cc: stable@vger.kernel.org # 6.4.x
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Link: https://patchwork.freedesktop.org/patch/msgid/ZK0Q5nbLyDO7kJa+@moroto
There are several issues in this code. The check at the start of the
loop:
if (user_len >= user_msg->len) {
This check does not ensure that we have enough space for the trans_hdr
(8 bytes). Instead the check needs to be:
if (user_len > user_msg->len - sizeof(*trans_hdr)) {
That subtraction is done as an unsigned long we want to avoid
negatives. Add a lower bound to the start of the function.
if (user_msg->len < sizeof(*trans_hdr))
There is a second integer underflow which can happen if
trans_hdr->len is zero inside the encode_passthrough() function.
memcpy(out_trans->data, in_trans->data, in_trans->hdr.len - sizeof(in_trans->hdr));
Instead of adding a check to encode_passthrough() it's better to check
in this central place. Add that check:
if (trans_hdr->len < sizeof(trans_hdr)
The final concern is that the "user_len + trans_hdr->len" might have an
integer overflow bug. Use size_add() to prevent that.
- if (user_len + trans_hdr->len > user_msg->len) {
+ if (size_add(user_len, trans_hdr->len) > user_msg->len) {
Fixes: 129776ac2e ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Cc: stable@vger.kernel.org # 6.4.x
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Link: https://patchwork.freedesktop.org/patch/msgid/9a0cb0c1-a974-4f10-bc8d-94437983639a@moroto.mountain
If msg_xfer() is unable to queue part of a NNC message because the MHI ring
is full, it will attempt to give the QSM some time to drain the queue.
However, if QSM fails to make any room, msg_xfer() will fail and tell the
caller to try again. This is problematic because part of the message may
have been committed to the ring and there is no mechanism to revoke that
content. This will cause QSM to receive a corrupt message.
The better way to do this is to check if the ring has enough space for the
entire message before committing any of the message. Since msg_xfer() is
under the cntl_mutex no one else can come in and consume the space.
Fixes: 129776ac2e ("accel/qaic: Add control path")
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230517193540.14323-6-quic_jhugo@quicinc.com
clang static analysis reports
drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
value returned to caller [core.uninitialized.UndefReturn]
return ret;
^~~~~~~~~~
From a code analysis of the function, the ret variable is only set some
of the time but is always returned. This suggests ret can return
uninitialized garbage. However BO allocation will ensure ret is always
set in reality.
Initialize ret to 0 to silence the warning.
Fixes: ff13be8303 ("accel/qaic: Add datapath")
Signed-off-by: Tom Rix <trix@redhat.com>
[jhugo: Reword commit text]
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230517165605.16770-1-quic_jhugo@quicinc.com
Some of the MHI channels for an AIC100 device need to be routed to
userspace so that userspace can communicate directly with QSM. The MHI
bus does not support this, and while the WWAN subsystem does (for the same
reasons), AIC100 is not a WWAN device. Also, MHI is not something that
other accelerators are expected to share, thus an accel subsystem function
that meets this usecase is unlikely.
Create a QAIC specific MHI userspace shim that exposes these channels.
Start with QAIC_SAHARA which is required to boot AIC100 and is consumed by
the kickstart application as documented in aic100.rst
Each AIC100 instance (currently, up to 16) in a system will create a
chardev for QAIC_SAHARA. This chardev will be found as
/dev/<mhi instance>_QAIC_SAHARA
For example - /dev/mhi0_QAIC_SAHARA
Signed-off-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>
Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Reviewed-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
Acked-by: Oded Gabbay <ogabbay@kernel.org>
Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/1679932497-30277-7-git-send-email-quic_jhugo@quicinc.com
