| Message ID | YsU4zCpaV7GBpHci@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ASoC: SOF: sof-client-probes: fix error codes in sof_probes_compr_copy() | expand |
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) {
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
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; }
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) {
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(-)