| Message ID | YsU4dYXYYVsfs92J@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: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; > } >
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; >> } >> >
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
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;
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
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;
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; }
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(-)