As the kcalloc may return NULL pointer,
it should be better to check the ishtp_dma_tx_map
before use in order to avoid NULL pointer dereference.
Fixes: 3703f53b99e4 ("HID: intel_ish-hid: ISH Transport layer")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/hid/intel-ish-hid/ishtp/dma-if.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/hid/intel-ish-hid/ishtp/dma-if.c b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
index 40554c8daca0..00046cbfd4ed 100644
--- a/drivers/hid/intel-ish-hid/ishtp/dma-if.c
+++ b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
@@ -104,6 +104,11 @@ void *ishtp_cl_get_dma_send_buf(struct ishtp_device *dev,
int required_slots = (size / DMA_SLOT_SIZE)
+ 1 * (size % DMA_SLOT_SIZE != 0);
+ if (!dev->ishtp_dma_tx_map) {
+ dev_err(dev->devc, "Fail to allocate Tx map\n");
+ return NULL;
+ }
+
spin_lock_irqsave(&dev->ishtp_dma_tx_lock, flags);
for (i = 0; i <= (dev->ishtp_dma_num_slots - required_slots); i++) {
free = 1;
@@ -150,6 +155,11 @@ void ishtp_cl_release_dma_acked_mem(struct ishtp_device *dev,
return;
}
+ if (!dev->ishtp_dma_tx_map) {
+ dev_err(dev->devc, "Fail to allocate Tx map\n");
+ return;
+ }
+
i = (msg_addr - dev->ishtp_host_dma_tx_buf) / DMA_SLOT_SIZE;
spin_lock_irqsave(&dev->ishtp_dma_tx_lock, flags);
for (j = 0; j < acked_slots; j++) {
--
2.25.1
On Tue, 22 Nov 2022, Jiasheng Jiang wrote:
> As the kcalloc may return NULL pointer,
> it should be better to check the ishtp_dma_tx_map
> before use in order to avoid NULL pointer dereference.
>
> Fixes: 3703f53b99e4 ("HID: intel_ish-hid: ISH Transport layer")
> Signed-off-by: Jiasheng Jiang <[email protected]>
Srinivas, can I get your Ack on this one, please?
I'd much prefer to perform the check right at the allocation time, but
that would need some more code refactoring (as
there is currently no way for ishtp_cl_alloc_dma_buf() to fail).
> ---
> drivers/hid/intel-ish-hid/ishtp/dma-if.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/dma-if.c b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> index 40554c8daca0..00046cbfd4ed 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> @@ -104,6 +104,11 @@ void *ishtp_cl_get_dma_send_buf(struct ishtp_device *dev,
> int required_slots = (size / DMA_SLOT_SIZE)
> + 1 * (size % DMA_SLOT_SIZE != 0);
>
> + if (!dev->ishtp_dma_tx_map) {
> + dev_err(dev->devc, "Fail to allocate Tx map\n");
I'd also suggest to use "Failed to ..." instead.
Thanks,
--
Jiri Kosina
SUSE Labs
On Tue, 2022-11-22 at 21:48 +0800, Jiasheng Jiang wrote:
> As the kcalloc may return NULL pointer,
> it should be better to check the ishtp_dma_tx_map
> before use in order to avoid NULL pointer dereference.
>
> Fixes: 3703f53b99e4 ("HID: intel_ish-hid: ISH Transport layer")
> Signed-off-by: Jiasheng Jiang <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
> ---
+Even Xu, We should do during alloc. Please try to submit a change for
that for later kernel rev as it will require error processing during
hbm dispatch.
> drivers/hid/intel-ish-hid/ishtp/dma-if.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> index 40554c8daca0..00046cbfd4ed 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> @@ -104,6 +104,11 @@ void *ishtp_cl_get_dma_send_buf(struct
> ishtp_device *dev,
> int required_slots = (size / DMA_SLOT_SIZE)
> + 1 * (size % DMA_SLOT_SIZE != 0);
>
> + if (!dev->ishtp_dma_tx_map) {
> + dev_err(dev->devc, "Fail to allocate Tx map\n");
> + return NULL;
> + }
> +
> spin_lock_irqsave(&dev->ishtp_dma_tx_lock, flags);
> for (i = 0; i <= (dev->ishtp_dma_num_slots - required_slots);
> i++) {
> free = 1;
> @@ -150,6 +155,11 @@ void ishtp_cl_release_dma_acked_mem(struct
> ishtp_device *dev,
> return;
> }
>
> + if (!dev->ishtp_dma_tx_map) {
> + dev_err(dev->devc, "Fail to allocate Tx map\n");
> + return;
> + }
> +
> i = (msg_addr - dev->ishtp_host_dma_tx_buf) / DMA_SLOT_SIZE;
> spin_lock_irqsave(&dev->ishtp_dma_tx_lock, flags);
> for (j = 0; j < acked_slots; j++) {
Yes, Srinivas, agree with you, the error handling should be added during allocation.
Will submit the patch for it.
Thanks!
Best Regards,
Even Xu
-----Original Message-----
From: srinivas pandruvada <[email protected]>
Sent: Wednesday, December 21, 2022 1:08 AM
To: Jiasheng Jiang <[email protected]>; [email protected]; [email protected]; Xu, Even <[email protected]>
Cc: [email protected]; [email protected]
Subject: Re: [PATCH] HID: intel_ish-hid: Add check for ishtp_dma_tx_map
On Tue, 2022-11-22 at 21:48 +0800, Jiasheng Jiang wrote:
> As the kcalloc may return NULL pointer, it should be better to check
> the ishtp_dma_tx_map before use in order to avoid NULL pointer
> dereference.
>
> Fixes: 3703f53b99e4 ("HID: intel_ish-hid: ISH Transport layer")
> Signed-off-by: Jiasheng Jiang <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
> ---
+Even Xu, We should do during alloc. Please try to submit a change for
that for later kernel rev as it will require error processing during hbm dispatch.
> drivers/hid/intel-ish-hid/ishtp/dma-if.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> index 40554c8daca0..00046cbfd4ed 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/dma-if.c
> @@ -104,6 +104,11 @@ void *ishtp_cl_get_dma_send_buf(struct
> ishtp_device *dev,
> int required_slots = (size / DMA_SLOT_SIZE)
> + 1 * (size % DMA_SLOT_SIZE != 0);
>
> + if (!dev->ishtp_dma_tx_map) {
> + dev_err(dev->devc, "Fail to allocate Tx map\n");
> + return NULL;
> + }
> +
> spin_lock_irqsave(&dev->ishtp_dma_tx_lock, flags);
> for (i = 0; i <= (dev->ishtp_dma_num_slots - required_slots);
> i++) {
> free = 1;
> @@ -150,6 +155,11 @@ void ishtp_cl_release_dma_acked_mem(struct
> ishtp_device *dev,
> return;
> }
>
> + if (!dev->ishtp_dma_tx_map) {
> + dev_err(dev->devc, "Fail to allocate Tx map\n");
> + return;
> + }
> +
> i = (msg_addr - dev->ishtp_host_dma_tx_buf) / DMA_SLOT_SIZE;
> spin_lock_irqsave(&dev->ishtp_dma_tx_lock, flags);
> for (j = 0; j < acked_slots; j++) {
On Wed, 21 Dec 2022, Xu, Even wrote:
> Yes, Srinivas, agree with you, the error handling should be added during allocation.
> Will submit the patch for it.
Thanks. Before that patch materializes though, I've queued Jiasheng's fix
as a band-aid for 6.2.
Thanks,
--
Jiri Kosina
SUSE Labs