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