HID: intel_ish-hid: Add check for ishtp_dma_tx_map

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 <jiasheng@iscas.ac.cn>
---
 drivers/hid/intel-ish-hid/ishtp/dma-if.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

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 <jiasheng@iscas.ac.cn>

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,

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 <jiasheng@iscas.ac.cn>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
+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 <srinivas.pandruvada@linux.intel.com> 
Sent: Wednesday, December 21, 2022 1:08 AM
To: Jiasheng Jiang <jiasheng@iscas.ac.cn>; jikos@kernel.org; benjamin.tissoires@redhat.com; Xu, Even <even.xu@intel.com>
Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
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 <jiasheng@iscas.ac.cn>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
+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,