[PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[Dan Carpenter]

This function tries to return the number of bytes that it was able to
copy to the user.  However, because there are multiple calls to
copy_to_user() in a row that means the bytes are not necessarily
consecutive so it's not useful.  Just return -EFAULT instead.

Fixes: 3dc0d7091778 ("ASoC: SOF: Convert the generic probe support to SOF client")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/sof/sof-client-probes.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 1f1ea93a7fbf..679bc7d371fc 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -385,7 +385,6 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
 	struct snd_compr_runtime *rtd = cstream->runtime;
 	unsigned int offset, n;
 	void *ptr;
-	int ret;
 
 	if (count > rtd->buffer_size)
 		count = rtd->buffer_size;
@@ -395,14 +394,15 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
 	n = rtd->buffer_size - offset;
 
 	if (count < n) {
-		ret = copy_to_user(buf, ptr, count);
+		if (copy_to_user(buf, ptr, count))
+			return -EFAULT;
 	} else {
-		ret = copy_to_user(buf, ptr, n);
-		ret += copy_to_user(buf + n, rtd->dma_area, count - n);
+		if (copy_to_user(buf, ptr, n))
+			return -EFAULT;
+		if (copy_to_user(buf + n, rtd->dma_area, count - n))
+			return -EFAULT;
 	}
 
-	if (ret)
-		return count - ret;
 	return count;
 }
 
-- 
2.35.1

[PATCH 2/2] ASoC: SOF: sof-client-probes: cleanup tokenize_input()

[Dan Carpenter]

The tokenize_input() function is cleaner if it uses strndup_user()
instead of simple_write_to_buffer().  The way it's written now, if
*ppos is non-zero then it returns -EIO but normally we would return
0 in that case.  It's easier to handle that in the callers.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/sof/sof-client-probes.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 679bc7d371fc..6c922b683f67 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -461,24 +461,17 @@ static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tk
 }
 
 static int tokenize_input(const char __user *from, size_t count,
-			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
+			  u32 **tkns, size_t *num_tkns)
 {
 	char *buf;
 	int ret;
 
-	buf = kmalloc(count + 1, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	ret = simple_write_to_buffer(buf, count, ppos, from, count);
-	if (ret != count) {
-		ret = ret >= 0 ? -EIO : ret;
-		goto exit;
-	}
+	buf = strndup_user(from, count + 1);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
 
-	buf[count] = '\0';
 	ret = strsplit_u32(buf, ",", tkns, num_tkns);
-exit:
+
 	kfree(buf);
 	return ret;
 }
@@ -552,12 +545,15 @@ sof_probes_dfs_points_write(struct file *file, const char __user *from,
 	u32 *tkns;
 	int ret, err;
 
+	if (*ppos)
+		return 0;
+
 	if (priv->extractor_stream_tag == SOF_PROBES_INVALID_NODE_ID) {
 		dev_warn(dev, "no extractor stream running\n");
 		return -ENOENT;
 	}
 
-	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	ret = tokenize_input(from, count, &tkns, &num_tkns);
 	if (ret < 0)
 		return ret;
 	bytes = sizeof(*tkns) * num_tkns;
@@ -607,12 +603,15 @@ sof_probes_dfs_points_remove_write(struct file *file, const char __user *from,
 	u32 *tkns;
 	int ret, err;
 
+	if (*ppos)
+		return 0;
+
 	if (priv->extractor_stream_tag == SOF_PROBES_INVALID_NODE_ID) {
 		dev_warn(dev, "no extractor stream running\n");
 		return -ENOENT;
 	}
 
-	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	ret = tokenize_input(from, count, &tkns, &num_tkns);
 	if (ret < 0)
 		return ret;
 	if (!num_tkns) {
-- 
2.35.1

Re: [PATCH 2/2] ASoC: SOF: sof-client-probes: cleanup tokenize_input()

[Péter Ujfalusi]

On 06/07/2022 10:25, Dan Carpenter wrote:
> The tokenize_input() function is cleaner if it uses strndup_user()
> instead of simple_write_to_buffer().  The way it's written now, if
> *ppos is non-zero then it returns -EIO but normally we would return
> 0 in that case.  It's easier to handle that in the callers.

This patch breaks the probe point settings:

# echo 52,1,0 > /sys/kernel/debug/sof/probe_points
-bash: echo: write error: Invalid argument

I did not looked for the exact reason, but something is not correct.

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  sound/soc/sof/sof-client-probes.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
> index 679bc7d371fc..6c922b683f67 100644
> --- a/sound/soc/sof/sof-client-probes.c
> +++ b/sound/soc/sof/sof-client-probes.c
> @@ -461,24 +461,17 @@ static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tk
>  }
>  
>  static int tokenize_input(const char __user *from, size_t count,
> -			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
> +			  u32 **tkns, size_t *num_tkns)
>  {
>  	char *buf;
>  	int ret;
>  
> -	buf = kmalloc(count + 1, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	ret = simple_write_to_buffer(buf, count, ppos, from, count);
> -	if (ret != count) {
> -		ret = ret >= 0 ? -EIO : ret;
> -		goto exit;
> -	}
> +	buf = strndup_user(from, count + 1);
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
>  
> -	buf[count] = '\0';
>  	ret = strsplit_u32(buf, ",", tkns, num_tkns);
> -exit:
> +
>  	kfree(buf);
>  	return ret;
>  }
> @@ -552,12 +545,15 @@ sof_probes_dfs_points_write(struct file *file, const char __user *from,
>  	u32 *tkns;
>  	int ret, err;
>  
> +	if (*ppos)
> +		return 0;
> +
>  	if (priv->extractor_stream_tag == SOF_PROBES_INVALID_NODE_ID) {
>  		dev_warn(dev, "no extractor stream running\n");
>  		return -ENOENT;
>  	}
>  
> -	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
> +	ret = tokenize_input(from, count, &tkns, &num_tkns);
>  	if (ret < 0)
>  		return ret;
>  	bytes = sizeof(*tkns) * num_tkns;
> @@ -607,12 +603,15 @@ sof_probes_dfs_points_remove_write(struct file *file, const char __user *from,
>  	u32 *tkns;
>  	int ret, err;
>  
> +	if (*ppos)
> +		return 0;
> +
>  	if (priv->extractor_stream_tag == SOF_PROBES_INVALID_NODE_ID) {
>  		dev_warn(dev, "no extractor stream running\n");
>  		return -ENOENT;
>  	}
>  
> -	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
> +	ret = tokenize_input(from, count, &tkns, &num_tkns);
>  	if (ret < 0)
>  		return ret;
>  	if (!num_tkns) {

-- 
Péter

Re: [PATCH 2/2] ASoC: SOF: sof-client-probes: cleanup tokenize_input()

[Dan Carpenter]

On Wed, Jul 06, 2022 at 12:27:49PM +0300, Péter Ujfalusi wrote:
> 
> 
> On 06/07/2022 10:25, Dan Carpenter wrote:
> > The tokenize_input() function is cleaner if it uses strndup_user()
> > instead of simple_write_to_buffer().  The way it's written now, if
> > *ppos is non-zero then it returns -EIO but normally we would return
> > 0 in that case.  It's easier to handle that in the callers.
> 
> This patch breaks the probe point settings:
> 
> # echo 52,1,0 > /sys/kernel/debug/sof/probe_points
> -bash: echo: write error: Invalid argument
> 
> I did not looked for the exact reason, but something is not correct.
> 

Crud...

Thanks for testing.

I used strndup_user() in a couple other patches today and I didn't
realize how strict it was.  I've NAKed my patches which used
strndup_user().  One of the patches was an infoleak patch so I'm going
to resend that using memdup_user() instead but let's just drop this one.

I guess another safer option would be to just always zero the buffers
going into simple_write_to_buffer()...

regards,
dan carpenter

Re: [PATCH 2/2] ASoC: SOF: sof-client-probes: cleanup tokenize_input()

[Cezary Rojewski]

On 2022-07-06 12:44 PM, Dan Carpenter wrote:
> On Wed, Jul 06, 2022 at 12:27:49PM +0300, Péter Ujfalusi wrote:
>>
>>
>> On 06/07/2022 10:25, Dan Carpenter wrote:
>>> The tokenize_input() function is cleaner if it uses strndup_user()
>>> instead of simple_write_to_buffer().  The way it's written now, if
>>> *ppos is non-zero then it returns -EIO but normally we would return
>>> 0 in that case.  It's easier to handle that in the callers.
>>
>> This patch breaks the probe point settings:
>>
>> # echo 52,1,0 > /sys/kernel/debug/sof/probe_points
>> -bash: echo: write error: Invalid argument
>>
>> I did not looked for the exact reason, but something is not correct.
>>
> 
> Crud...
> 
> Thanks for testing.
> 
> I used strndup_user() in a couple other patches today and I didn't
> realize how strict it was.  I've NAKed my patches which used
> strndup_user().  One of the patches was an infoleak patch so I'm going
> to resend that using memdup_user() instead but let's just drop this one.
> 
> I guess another safer option would be to just always zero the buffers
> going into simple_write_to_buffer()...
> 
> regards,
> dan carpenter
> 


Hello,

Indeed the strsplit_u32() contains some bugs - tokenize_input() needs no 
fixes if I'm not mistaken though.
It seems I did not realize the bugs were not fixed. As the avs-driver 
makes use of probes too and these are being tested there regularly the 
team did notice the problems. Below is the implementation. I'm saying 
this as the plan is to move both strsplit_u32() and tokenize_input() 
into the common code so it can be re-used by both drivers. Will send the 
patches soon :)


Regards,
Czarek


static int
strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t 
*num_tkns)
{
         size_t max_count = 32;
         size_t count = 0;
         char *s, **p;
         u32 *buf, *tmp;
         int ret = 0;

         p = (char **)&str;
         *tkns = NULL;
         *num_tkns = 0;

         buf = kcalloc(max_count, sizeof(*buf), GFP_KERNEL);
         if (!buf)
                 return -ENOMEM;

         while ((s = strsep(p, delim)) != NULL) {
                 ret = kstrtouint(s, 0, buf + count);
                 if (ret)
                         goto free_buf;

                 if (++count > max_count) {
                         max_count *= 2;
                         tmp = krealloc(buf, max_count * sizeof(*buf), 
GFP_KERNEL);
                         if (!tmp) {
                                 ret = -ENOMEM;
                                 goto free_buf;
                         }
                         buf = tmp;
                 }
         }

         if (!count)
                 goto free_buf;
         *tkns = kmemdup(buf, count * sizeof(*buf), GFP_KERNEL);
         if (*tkns == NULL) {
                 ret = -ENOMEM;
                 goto free_buf;
         }
         *num_tkns = count;

free_buf:
         kfree(buf);
         return ret;
}

Re: [PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[Péter Ujfalusi]

On 06/07/2022 10:23, Dan Carpenter wrote:
> This function tries to return the number of bytes that it was able to
> copy to the user.  However, because there are multiple calls to
> copy_to_user() in a row that means the bytes are not necessarily
> consecutive so it's not useful.  Just return -EFAULT instead.

The function is copying data from a circular buffer to a use buffer.
The single copy_to_user() is used when we don't have wrapping, the
'double' copy_to_user() is when we wrap, so first copy is from the end
of the buffer then we copy the data from the start of the buffer to get
all data.

> Fixes: 3dc0d7091778 ("ASoC: SOF: Convert the generic probe support to SOF client")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  sound/soc/sof/sof-client-probes.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
> index 1f1ea93a7fbf..679bc7d371fc 100644
> --- a/sound/soc/sof/sof-client-probes.c
> +++ b/sound/soc/sof/sof-client-probes.c
> @@ -385,7 +385,6 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
>  	struct snd_compr_runtime *rtd = cstream->runtime;
>  	unsigned int offset, n;
>  	void *ptr;
> -	int ret;
>  
>  	if (count > rtd->buffer_size)
>  		count = rtd->buffer_size;
> @@ -395,14 +394,15 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
>  	n = rtd->buffer_size - offset;
>  
>  	if (count < n) {
> -		ret = copy_to_user(buf, ptr, count);
> +		if (copy_to_user(buf, ptr, count))
> +			return -EFAULT;
>  	} else {
> -		ret = copy_to_user(buf, ptr, n);
> -		ret += copy_to_user(buf + n, rtd->dma_area, count - n);
> +		if (copy_to_user(buf, ptr, n))
> +			return -EFAULT;
> +		if (copy_to_user(buf + n, rtd->dma_area, count - n))
> +			return -EFAULT;
>  	}
>  
> -	if (ret)
> -		return count - ret;
>  	return count;
>  }
>  

-- 
Péter

Re: [PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[Péter Ujfalusi]

On 06/07/2022 12:05, Péter Ujfalusi wrote:
> 
> 
> On 06/07/2022 10:23, Dan Carpenter wrote:
>> This function tries to return the number of bytes that it was able to
>> copy to the user.  However, because there are multiple calls to
>> copy_to_user() in a row that means the bytes are not necessarily
>> consecutive so it's not useful.  Just return -EFAULT instead.
> 
> The function is copying data from a circular buffer to a use buffer.
> The single copy_to_user() is used when we don't have wrapping, the
> 'double' copy_to_user() is when we wrap, so first copy is from the end
> of the buffer then we copy the data from the start of the buffer to get
> all data.

What I wanted to say is that the original code is correct, this patch
would break the functionality.

> 
>> Fixes: 3dc0d7091778 ("ASoC: SOF: Convert the generic probe support to SOF client")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>>  sound/soc/sof/sof-client-probes.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
>> index 1f1ea93a7fbf..679bc7d371fc 100644
>> --- a/sound/soc/sof/sof-client-probes.c
>> +++ b/sound/soc/sof/sof-client-probes.c
>> @@ -385,7 +385,6 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
>>  	struct snd_compr_runtime *rtd = cstream->runtime;
>>  	unsigned int offset, n;
>>  	void *ptr;
>> -	int ret;
>>  
>>  	if (count > rtd->buffer_size)
>>  		count = rtd->buffer_size;
>> @@ -395,14 +394,15 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
>>  	n = rtd->buffer_size - offset;
>>  
>>  	if (count < n) {
>> -		ret = copy_to_user(buf, ptr, count);
>> +		if (copy_to_user(buf, ptr, count))
>> +			return -EFAULT;
>>  	} else {
>> -		ret = copy_to_user(buf, ptr, n);
>> -		ret += copy_to_user(buf + n, rtd->dma_area, count - n);
>> +		if (copy_to_user(buf, ptr, n))
>> +			return -EFAULT;
>> +		if (copy_to_user(buf + n, rtd->dma_area, count - n))
>> +			return -EFAULT;
>>  	}
>>  
>> -	if (ret)
>> -		return count - ret;
>>  	return count;
>>  }
>>  
> 

-- 
Péter

Re: [PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[Dan Carpenter]

On Wed, Jul 06, 2022 at 12:05:37PM +0300, Péter Ujfalusi wrote:
> 
> 
> On 06/07/2022 10:23, Dan Carpenter wrote:
> > This function tries to return the number of bytes that it was able to
> > copy to the user.  However, because there are multiple calls to
> > copy_to_user() in a row that means the bytes are not necessarily
> > consecutive so it's not useful.  Just return -EFAULT instead.
> 
> The function is copying data from a circular buffer to a use buffer.
> The single copy_to_user() is used when we don't have wrapping, the
> 'double' copy_to_user() is when we wrap, so first copy is from the end
> of the buffer then we copy the data from the start of the buffer to get
> all data.

Ok.  But the bugs in the original code are real.  I will resend.

regards,
dan carpenter

Re: [PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[Dan Carpenter]

On Wed, Jul 06, 2022 at 01:21:59PM +0300, Dan Carpenter wrote:
> On Wed, Jul 06, 2022 at 12:05:37PM +0300, Péter Ujfalusi wrote:
> > 
> > 
> > On 06/07/2022 10:23, Dan Carpenter wrote:
> > > This function tries to return the number of bytes that it was able to
> > > copy to the user.  However, because there are multiple calls to
> > > copy_to_user() in a row that means the bytes are not necessarily
> > > consecutive so it's not useful.  Just return -EFAULT instead.
> > 
> > The function is copying data from a circular buffer to a use buffer.
> > The single copy_to_user() is used when we don't have wrapping, the
> > 'double' copy_to_user() is when we wrap, so first copy is from the end
> > of the buffer then we copy the data from the start of the buffer to get
> > all data.
> 
> Ok.  But the bugs in the original code are real.  I will resend.

Actually that's not true.  The bugs in the original code are something
that only affect users who deserve it?  I might not resend.  A fix would
look something like below?

regards,
dan carpenter

diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 1f1ea93a7fbf..32fa3186c295 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -398,9 +398,14 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
 		ret = copy_to_user(buf, ptr, count);
 	} else {
 		ret = copy_to_user(buf, ptr, n);
-		ret += copy_to_user(buf + n, rtd->dma_area, count - n);
+		if (ret) {
+			ret += count - n;
+			goto done;
+		}
+		ret = copy_to_user(buf + n, rtd->dma_area, count - n);
 	}
 
+done:
 	if (ret)
 		return count - ret;
 	return count;

Re: [PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[Dan Carpenter]

On Wed, Jul 06, 2022 at 01:31:39PM +0300, Dan Carpenter wrote:
> On Wed, Jul 06, 2022 at 01:21:59PM +0300, Dan Carpenter wrote:
> > On Wed, Jul 06, 2022 at 12:05:37PM +0300, Péter Ujfalusi wrote:
> > > 
> > > 
> > > On 06/07/2022 10:23, Dan Carpenter wrote:
> > > > This function tries to return the number of bytes that it was able to
> > > > copy to the user.  However, because there are multiple calls to
> > > > copy_to_user() in a row that means the bytes are not necessarily
> > > > consecutive so it's not useful.  Just return -EFAULT instead.
> > > 
> > > The function is copying data from a circular buffer to a use buffer.
> > > The single copy_to_user() is used when we don't have wrapping, the
> > > 'double' copy_to_user() is when we wrap, so first copy is from the end
> > > of the buffer then we copy the data from the start of the buffer to get
> > > all data.
> > 
> > Ok.  But the bugs in the original code are real.  I will resend.
> 
> Actually that's not true.  The bugs in the original code are something
> that only affect users who deserve it?

Yeah.  Never mind.  If you set up user space so the first copy fails and
the second succeeds then you deserve it.

regards,
dan carpenter

Re: [PATCH 1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy()

[Péter Ujfalusi]

On 06/07/2022 13:31, Dan Carpenter wrote:
> On Wed, Jul 06, 2022 at 01:21:59PM +0300, Dan Carpenter wrote:
>> On Wed, Jul 06, 2022 at 12:05:37PM +0300, Péter Ujfalusi wrote:
>>>
>>>
>>> On 06/07/2022 10:23, Dan Carpenter wrote:
>>>> This function tries to return the number of bytes that it was able to
>>>> copy to the user.  However, because there are multiple calls to
>>>> copy_to_user() in a row that means the bytes are not necessarily
>>>> consecutive so it's not useful.  Just return -EFAULT instead.
>>>
>>> The function is copying data from a circular buffer to a use buffer.
>>> The single copy_to_user() is used when we don't have wrapping, the
>>> 'double' copy_to_user() is when we wrap, so first copy is from the end
>>> of the buffer then we copy the data from the start of the buffer to get
>>> all data.
>>
>> Ok.  But the bugs in the original code are real.  I will resend.
> 
> Actually that's not true.  The bugs in the original code are something
> that only affect users who deserve it?  I might not resend.  A fix would
> look something like below?
> 
> regards,
> dan carpenter
> 
> diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
> index 1f1ea93a7fbf..32fa3186c295 100644
> --- a/sound/soc/sof/sof-client-probes.c
> +++ b/sound/soc/sof/sof-client-probes.c
> @@ -398,9 +398,14 @@ static int sof_probes_compr_copy(struct snd_soc_component *component,
>  		ret = copy_to_user(buf, ptr, count);
>  	} else {
>  		ret = copy_to_user(buf, ptr, n);
> -		ret += copy_to_user(buf + n, rtd->dma_area, count - n);
> +		if (ret) {
> +			ret += count - n;
> +			goto done;
> +		}
> +		ret = copy_to_user(buf + n, rtd->dma_area, count - n);

I think this should work, can you please resend it?

>  	}
>  
> +done:
>  	if (ret)
>  		return count - ret;
>  	return count;

-- 
Péter