[RFC PATCH 0/1] Sanity check buffer callback

[RFC PATCH 0/1] Sanity check buffer callback

[Nuno Sá]

When going through the code of the buffer-cb interface and all it's
users, I realized that the stm32_adfsdm driver is calling
`iio_channel_get_all_cb()` with NULL for the cb. After going a bit
trough the stm drivers, it looks like this is actually intentional.
However, it is clear that we have a clear/direct route here for a NULL
pointer dereference. I'm being lazy in this RFC and just doing a
sanity check in the `iio_buffer_cb_store_to()` so that we don't have to
change the stm driver... The point is just to bring this up and see if
we want to do something about this.

To be clear, the way I think this should go is just to return -EINVAL in
`iio_channel_get_all_cb()` if a NULL ptr is passed. Whats the point of a
buffer-cb if cb is NULL, right? This would naturally break the stm
driver, but I guess we could just define a dummy handler there that
would not be used (or could the HW consumer be an option here?)...

Thoughts?

Nuno Sá (1):
  iio: buffer: Sanity check buffer callback

 drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.29.2

[RFC PATCH 1/1] iio: buffer: Sanity check buffer callback

[Nuno Sá]

Make sure that a callback is actually present before trying to call
it...

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
index 47c96f7f4976..b4385f59399d 100644
--- a/drivers/iio/buffer/industrialio-buffer-cb.c
+++ b/drivers/iio/buffer/industrialio-buffer-cb.c
@@ -27,6 +27,10 @@ static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer)
 static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
 {
 	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
+
+	if (!cb_buff->cb)
+		return 0;
+
 	return cb_buff->cb(data, cb_buff->private);
 }
 
-- 
2.29.2

Re: [RFC PATCH 1/1] iio: buffer: Sanity check buffer callback

[Lars-Peter Clausen]

On 11/12/20 4:13 PM, Nuno Sá wrote:
> Make sure that a callback is actually present before trying to call
> it...
>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>   drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
> index 47c96f7f4976..b4385f59399d 100644
> --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> @@ -27,6 +27,10 @@ static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer)
>   static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data)
>   {
>   	struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> +
> +	if (!cb_buff->cb)
> +		return 0;
> +
I think it makes more sense to check this once when the buffer is 
registered instead of every time we want to call the function.

Re: [RFC PATCH 1/1] iio: buffer: Sanity check buffer callback

[Lars-Peter Clausen]

On 11/12/20 5:15 PM, Lars-Peter Clausen wrote:
> On 11/12/20 4:13 PM, Nuno Sá wrote:
>> Make sure that a callback is actually present before trying to call
>> it...
>>
>> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
>> ---
>>   drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c 
>> b/drivers/iio/buffer/industrialio-buffer-cb.c
>> index 47c96f7f4976..b4385f59399d 100644
>> --- a/drivers/iio/buffer/industrialio-buffer-cb.c
>> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
>> @@ -27,6 +27,10 @@ static struct iio_cb_buffer 
>> *buffer_to_cb_buffer(struct iio_buffer *buffer)
>>   static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const 
>> void *data)
>>   {
>>       struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
>> +
>> +    if (!cb_buff->cb)
>> +        return 0;
>> +
> I think it makes more sense to check this once when the buffer is 
> registered instead of every time we want to call the function.

Sorry, should have read the cover letter more carefully. What you write 
there makes sense.

Re: [RFC PATCH 0/1] Sanity check buffer callback

[Jonathan Cameron]

On Thu, 12 Nov 2020 16:13:33 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> When going through the code of the buffer-cb interface and all it's
> users, I realized that the stm32_adfsdm driver is calling
> `iio_channel_get_all_cb()` with NULL for the cb. After going a bit
> trough the stm drivers, it looks like this is actually intentional.
> However, it is clear that we have a clear/direct route here for a NULL
> pointer dereference. I'm being lazy in this RFC and just doing a
> sanity check in the `iio_buffer_cb_store_to()` so that we don't have to
> change the stm driver... The point is just to bring this up and see if
> we want to do something about this.
> 
> To be clear, the way I think this should go is just to return -EINVAL in
> `iio_channel_get_all_cb()` if a NULL ptr is passed. Whats the point of a
> buffer-cb if cb is NULL, right? This would naturally break the stm
> driver, but I guess we could just define a dummy handler there that
> would not be used (or could the HW consumer be an option here?)...
> 
> Thoughts?

Good description thanks.  I think you are right and better option is
to return -EINVAL in iio_channel_get_all_cb() and add a dummy  handler to the
stm driver.

cc Arnaud and Olivier to see if they are fine with the dummy handler.
(with appropriate comment on why it is there).

Jonathan


> 
> Nuno Sá (1):
>   iio: buffer: Sanity check buffer callback
> 
>  drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Re: [RFC PATCH 0/1] Sanity check buffer callback

[Olivier MOYSAN]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

On 11/14/20 5:33 PM, Jonathan Cameron wrote:
> On Thu, 12 Nov 2020 16:13:33 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
>> When going through the code of the buffer-cb interface and all it's
>> users, I realized that the stm32_adfsdm driver is calling
>> `iio_channel_get_all_cb()` with NULL for the cb. After going a bit
>> trough the stm drivers, it looks like this is actually intentional.
>> However, it is clear that we have a clear/direct route here for a NULL
>> pointer dereference. I'm being lazy in this RFC and just doing a
>> sanity check in the `iio_buffer_cb_store_to()` so that we don't have to
>> change the stm driver... The point is just to bring this up and see if
>> we want to do something about this.
>>
>> To be clear, the way I think this should go is just to return -EINVAL in
>> `iio_channel_get_all_cb()` if a NULL ptr is passed. Whats the point of a
>> buffer-cb if cb is NULL, right? This would naturally break the stm
>> driver, but I guess we could just define a dummy handler there that
>> would not be used (or could the HW consumer be an option here?)...
>>
>> Thoughts?
> 
> Good description thanks.  I think you are right and better option is
> to return -EINVAL in iio_channel_get_all_cb() and add a dummy  handler to the
> stm driver.
> 
> cc Arnaud and Olivier to see if they are fine with the dummy handler.
> (with appropriate comment on why it is there).
> 
> Jonathan
> 
> 

Hi Jonathan,

Thanks for your notice. Arnaud and myself agree with this solution.
I attach to this mail a patch which implements this dummy handler
and can be added to Nuno's patchset, to avoid a regression on stm driver.

Regards,
Olivier

>>
>> Nuno Sá (1):
>>    iio: buffer: Sanity check buffer callback
>>
>>   drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ASoC-stm32-dfsdm-add-stm32_adfsdm_dummy_cb-callback.patch --]
[-- Type: text/x-patch; name="0001-ASoC-stm32-dfsdm-add-stm32_adfsdm_dummy_cb-callback.patch", Size: 1787 bytes --]

From 98d3008bb2cdee75b2efb42a7d3a5af9bed47e9c Mon Sep 17 00:00:00 2001
From: Olivier Moysan <olivier.moysan@st.com>
Date: Mon, 16 Nov 2020 17:23:43 +0100
Subject: [PATCH] ASoC: stm32: dfsdm: add stm32_adfsdm_dummy_cb() callback

Adapt STM32 DFSDM driver to a change in iio_channel_get_all_cb() API.
The callback pointer becomes a requested parameter of this API,
so add a dummy callback to be given as parameter of this function.
However, the stm32_dfsdm_get_buff_cb() API is still used instead,
to optimize DMA transfers.

Signed-off-by: Olivier Moysan <olivier.moysan@st.com>
---
 sound/soc/stm/stm32_adfsdm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/sound/soc/stm/stm32_adfsdm.c b/sound/soc/stm/stm32_adfsdm.c
index c4031988f981..47fae8dd20b4 100644
--- a/sound/soc/stm/stm32_adfsdm.c
+++ b/sound/soc/stm/stm32_adfsdm.c
@@ -293,6 +293,16 @@ static int stm32_adfsdm_pcm_new(struct snd_soc_component *component,
 	return 0;
 }
 
+static int stm32_adfsdm_dummy_cb(const void *data, void *private)
+{
+	/*
+	 * This dummmy callback is requested by iio_channel_get_all_cb() API,
+	 * but the stm32_dfsdm_get_buff_cb() API is used instead, to optimize
+	 * DMA transfers.
+	 */
+	return 0;
+}
+
 static struct snd_soc_component_driver stm32_adfsdm_soc_platform = {
 	.open		= stm32_adfsdm_pcm_open,
 	.close		= stm32_adfsdm_pcm_close,
@@ -335,7 +345,7 @@ static int stm32_adfsdm_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->iio_ch))
 		return PTR_ERR(priv->iio_ch);
 
-	priv->iio_cb = iio_channel_get_all_cb(&pdev->dev, NULL, NULL);
+	priv->iio_cb = iio_channel_get_all_cb(&pdev->dev, &stm32_adfsdm_dummy_cb, NULL);
 	if (IS_ERR(priv->iio_cb))
 		return PTR_ERR(priv->iio_cb);
 
-- 
2.17.1