linux.git/drivers/accel/qaic, branch v6.5

accel/qaic: Clean up integer overflow checking in map_user_pages()

2023-08-15T15:51:13+00:00

The encode_dma() function has some validation on in_trans->size but it
would be more clear to move those checks to find_and_map_user_pages().

The encode_dma() had two checks:

	if (in_trans->addr + in_trans->size < in_trans->addr || !in_trans->size)
		return -EINVAL;

The in_trans->addr variable is the starting address.  The in_trans->size
variable is the total size of the transfer.  The transfer can occur in
parts and the resources->xferred_dma_size tracks how many bytes we have
already transferred.

This patch introduces a new variable "remaining" which represents the
amount we want to transfer (in_trans->size) minus the amount we have
already transferred (resources->xferred_dma_size).

I have modified the check for if in_trans->size is zero to instead check
if in_trans->size is less than resources->xferred_dma_size.  If we have
already transferred more bytes than in_trans->size then there are negative
bytes remaining which doesn't make sense.  If there are zero bytes
remaining to be copied, just return success.

The check in encode_dma() checked that "addr + size" could not overflow
and barring a driver bug that should work, but it's easier to check if
we do this in parts.  First check that "in_trans->addr +
resources->xferred_dma_size" is safe.  Then check that "xfer_start_addr +
remaining" is safe.

My final concern was that we are dealing with u64 values but on 32bit
systems the kmalloc() function will truncate the sizes to 32 bits.  So
I calculated "total = in_trans->size + offset_in_page(xfer_start_addr);"
and returned -EINVAL if it were >= SIZE_MAX.  This will not affect 64bit
systems.

Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter 
Reviewed-by: Jeffrey Hugo 
Reviewed-by: Carl Vanderlip 
Signed-off-by: Jeffrey Hugo 
Link: https://patchwork.freedesktop.org/patch/msgid/24d3348b-25ac-4c1b-b171-9dae7c43e4e0@moroto.mountain

accel/qaic: Fix slicing memory leak

2023-08-15T15:50:07+00:00

The temporary buffer storing slicing configuration data from user is only
freed on error.  This is a memory leak.  Free the buffer unconditionally.

Fixes: ff13be830333 ("accel/qaic: Add datapath")
Signed-off-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Carl Vanderlip 
Reviewed-by: Jeffrey Hugo 
Signed-off-by: Jeffrey Hugo 
Link: https://patchwork.freedesktop.org/patch/msgid/20230802145937.14827-1-quic_jhugo@quicinc.com

accel/qaic: Fix a leak in map_user_pages()

2023-07-14T16:14:58+00:00

If get_user_pages_fast() allocates some pages but not as many as we
wanted, then the current code leaks those pages.  Call put_page() on
the pages before returning.

Fixes: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Jeffrey Hugo 
Reviewed-by: Dafna Hirschfeld 
Cc: stable@vger.kernel.org # 6.4.x
Signed-off-by: Jeffrey Hugo 
Link: https://patchwork.freedesktop.org/patch/msgid/ZK0Q+ZuONTsBG+1T@moroto

accel/qaic: Add consistent integer overflow checks

2023-07-14T16:12:05+00:00

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 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Jeffrey Hugo 
Cc: stable@vger.kernel.org # 6.4.x
[jhugo: tweak if in encode_dma() to match existing style]
Signed-off-by: Jeffrey Hugo 
Link: https://patchwork.freedesktop.org/patch/msgid/ZK0Q7IsPkj6WSCcL@moroto

accel/qaic: tighten bounds checking in decode_message()

2023-07-14T16:03:09+00:00

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 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Jeffrey Hugo 
Cc: stable@vger.kernel.org # 6.4.x
Signed-off-by: Jeffrey Hugo 
Link: https://patchwork.freedesktop.org/patch/msgid/ZK0Q5nbLyDO7kJa+@moroto

accel/qaic: tighten bounds checking in encode_message()

2023-07-14T16:00:45+00:00

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: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Dan Carpenter 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Jeffrey Hugo 
Cc: stable@vger.kernel.org # 6.4.x
Signed-off-by: Jeffrey Hugo 
Link: https://patchwork.freedesktop.org/patch/msgid/9a0cb0c1-a974-4f10-bc8d-94437983639a@moroto.mountain

accel/qaic: Call DRM helper function to destroy prime GEM

2023-06-20T14:07:29+00:00

smatch warning:
	drivers/accel/qaic/qaic_data.c:620 qaic_free_object() error:
		dereferencing freed memory 'obj->import_attach'

obj->import_attach is detached and freed using dma_buf_detach().
But used after free to decrease the dmabuf ref count using
dma_buf_put().

drm_prime_gem_destroy() handles this issue and performs the proper clean
up instead of open coding it in the driver.

Fixes: ff13be830333 ("accel/qaic: Add datapath")
Reported-by: Sukrut Bellary 
Closes: https://lore.kernel.org/all/20230610021200.377452-1-sukrut.bellary@linux.com/
Suggested-by: Christian König 
Signed-off-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Carl Vanderlip 
Reviewed-by: Jeffrey Hugo 
Signed-off-by: Jeffrey Hugo 
Reviewed-by: Christian König 
Link: https://patchwork.freedesktop.org/patch/msgid/20230614161528.11710-1-quic_jhugo@quicinc.com

accel/qaic: Fix NULL pointer deref in qaic_destroy_drm_device()

2023-06-09T17:07:28+00:00

If qaic_destroy_drm_device() is called before the device has fully
initialized it will cause a NULL pointer dereference as the drm device
has not yet been created. Fix this with a NULL check.

Fixes: c501ca23a6a3 ("accel/qaic: Add uapi and core driver file")
Signed-off-by: Jeffrey Hugo 
Reviewed-by: Carl Vanderlip 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Link: https://patchwork.freedesktop.org/patch/msgid/20230602210440.8411-3-quic_jhugo@quicinc.com

accel/qaic: Free user handle on interrupted mutex

2023-06-09T17:06:33+00:00

After user handle is allocated, if mutex is interrupted, we do not free
the user handle and return an error. Kref had been initialized, but not
added to users list, so device teardown would also not call free_usr.

Fixes: c501ca23a6a3 ("accel/qaic: Add uapi and core driver file")
Signed-off-by: Carl Vanderlip 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Jeffrey Hugo 
Signed-off-by: Jeffrey Hugo 
Link: https://patchwork.freedesktop.org/patch/msgid/20230602210440.8411-2-quic_jhugo@quicinc.com

accel/qaic: Fix NNC message corruption

2023-05-23T15:51:38+00:00

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: 129776ac2e38 ("accel/qaic: Add control path")
Signed-off-by: Jeffrey Hugo 
Reviewed-by: Pranjal Ramajor Asha Kanojiya 
Reviewed-by: Carl Vanderlip 
Link: https://patchwork.freedesktop.org/patch/msgid/20230517193540.14323-6-quic_jhugo@quicinc.com