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

Cezary Rojewski <cezary.rojewski@xxxxxxxxx> · Wed, 6 Jul 2022 12:56:57 +0200

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;
}