* [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c
@ 2024-12-26 18:29 Al Viro
2024-12-26 19:00 ` Jaroslav Kysela
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2024-12-26 18:29 UTC (permalink / raw)
To: linux-fsdevel
Cc: Jaroslav Kysela, Amadeusz Sławiński, Takashi Iwai,
linux-sound, Vinod Koul
[please, review and test]
1) uses of dma_buf_get() are racy - as soon as a reference has been inserted
into descriptor table, it's fair game for dup2(), etc.; we can no longer
count upon that descriptor resolving to the same file. get_dma_buf() should
be used instead (and before the insertions into table, lest we get hit with
use-after-free).
2) there's no cleanup possible past the successful dma_buf_fd() - again,
once it's in descriptor table, that's it. Just do fd_install() when
we are past all failure exits. As it is, failure in the second
dma_buf_fd() leads to task->input->file reference moved into
descriptor table *and* dropped by dma_buf_put() from snd_compr_task_free()
after goto cleanup. I.e. a dangling pointer left in descriptor table.
Frankly, dma_buf_fd() is an attractive nuisance - it's very easy to get
wrong.
Fixes: 04177158cf98 "ALSA: compress_offload: introduce accel operation mode"
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 86ed2fbee0c8..97526957d629 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -1026,6 +1026,7 @@ static int snd_compr_task_new(struct snd_compr_stream *stream, struct snd_compr_
{
struct snd_compr_task_runtime *task;
int retval;
+ int fd[2];
if (stream->runtime->total_tasks >= stream->runtime->fragments)
return -EBUSY;
@@ -1039,19 +1040,31 @@ static int snd_compr_task_new(struct snd_compr_stream *stream, struct snd_compr_
retval = stream->ops->task_create(stream, task);
if (retval < 0)
goto cleanup;
- utask->input_fd = dma_buf_fd(task->input, O_WRONLY|O_CLOEXEC);
- if (utask->input_fd < 0) {
- retval = utask->input_fd;
+ if (!task->input || !task->input->file ||
+ !task->output || !task->output->file) {
+ retval = -EINVAL;
goto cleanup;
}
- utask->output_fd = dma_buf_fd(task->output, O_RDONLY|O_CLOEXEC);
- if (utask->output_fd < 0) {
- retval = utask->output_fd;
+
+ fd[0] = get_unused_fd_flags(O_CLOEXEC);
+ if (unlikely(fd[0] < 0)) {
+ retval = fd[0];
+ goto cleanup;
+ }
+ fd[1] = get_unused_fd_flags(O_CLOEXEC);
+ if (unlikely(fd[1] < 0)) {
+ put_unused_fd(fd[0]);
+ retval = fd[1];
goto cleanup;
}
+
/* keep dmabuf reference until freed with task free ioctl */
- dma_buf_get(utask->input_fd);
- dma_buf_get(utask->output_fd);
+ get_dma_buf(task->input);
+ get_dma_buf(task->output);
+
+ fd_install(fd[0], task->input->file);
+ fd_install(fd[1], task->output->file);
+
list_add_tail(&task->list, &stream->runtime->tasks);
stream->runtime->total_tasks++;
return 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c
2024-12-26 18:29 [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c Al Viro
@ 2024-12-26 19:00 ` Jaroslav Kysela
2024-12-26 21:31 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Jaroslav Kysela @ 2024-12-26 19:00 UTC (permalink / raw)
To: Al Viro, linux-fsdevel
Cc: Amadeusz Sławiński, Takashi Iwai, linux-sound,
Vinod Koul
On 26. 12. 24 19:29, Al Viro wrote:
> [please, review and test]
>
> 1) uses of dma_buf_get() are racy - as soon as a reference has been inserted
> into descriptor table, it's fair game for dup2(), etc.; we can no longer
> count upon that descriptor resolving to the same file. get_dma_buf() should
> be used instead (and before the insertions into table, lest we get hit with
> use-after-free).
>
> 2) there's no cleanup possible past the successful dma_buf_fd() - again,
> once it's in descriptor table, that's it. Just do fd_install() when
> we are past all failure exits. As it is, failure in the second
> dma_buf_fd() leads to task->input->file reference moved into
> descriptor table *and* dropped by dma_buf_put() from snd_compr_task_free()
> after goto cleanup. I.e. a dangling pointer left in descriptor table.
>
> Frankly, dma_buf_fd() is an attractive nuisance - it's very easy to get
> wrong.
>
> Fixes: 04177158cf98 "ALSA: compress_offload: introduce accel operation mode"
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Hi,
I already made almost similar patch:
https://lore.kernel.org/linux-sound/20241217100726.732863-1-perex@perex.cz/
Jaroslav
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c
2024-12-26 19:00 ` Jaroslav Kysela
@ 2024-12-26 21:31 ` Al Viro
2024-12-26 22:17 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2024-12-26 21:31 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: linux-fsdevel, Amadeusz Sławiński, Takashi Iwai,
linux-sound, Vinod Koul
On Thu, Dec 26, 2024 at 08:00:18PM +0100, Jaroslav Kysela wrote:
> I already made almost similar patch:
>
> https://lore.kernel.org/linux-sound/20241217100726.732863-1-perex@perex.cz/
Umm... The only problem with your variant is that dma_buf_get()
is wrong here - it should be get_dma_buf() on actual objects,
and it should be done before fd_install().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c
2024-12-26 21:31 ` Al Viro
@ 2024-12-26 22:17 ` Al Viro
2024-12-29 8:35 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2024-12-26 22:17 UTC (permalink / raw)
To: Jaroslav Kysela
Cc: linux-fsdevel, Amadeusz Sławiński, Takashi Iwai,
linux-sound, Vinod Koul
On Thu, Dec 26, 2024 at 09:31:22PM +0000, Al Viro wrote:
> On Thu, Dec 26, 2024 at 08:00:18PM +0100, Jaroslav Kysela wrote:
>
> > I already made almost similar patch:
> >
> > https://lore.kernel.org/linux-sound/20241217100726.732863-1-perex@perex.cz/
>
> Umm... The only problem with your variant is that dma_buf_get()
> is wrong here - it should be get_dma_buf() on actual objects,
> and it should be done before fd_install().
Incremental on top of what just got merged into mainline:
Grab the references to dmabuf before moving them into descriptor
table - trying to do that by descriptor afterwards might end up getting
a different object, with a dangling reference left in task->{input,output}
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index edf5aadf38e5..543c7f525f84 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -1053,13 +1053,13 @@ static int snd_compr_task_new(struct snd_compr_stream *stream, struct snd_compr_
put_unused_fd(fd_i);
goto cleanup;
}
+ /* keep dmabuf reference until freed with task free ioctl */
+ get_dma_buf(task->input);
+ get_dma_buf(task->output);
fd_install(fd_i, task->input->file);
fd_install(fd_o, task->output->file);
utask->input_fd = fd_i;
utask->output_fd = fd_o;
- /* keep dmabuf reference until freed with task free ioctl */
- dma_buf_get(utask->input_fd);
- dma_buf_get(utask->output_fd);
list_add_tail(&task->list, &stream->runtime->tasks);
stream->runtime->total_tasks++;
return 0;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c
2024-12-26 22:17 ` Al Viro
@ 2024-12-29 8:35 ` Takashi Iwai
2024-12-29 18:53 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2024-12-29 8:35 UTC (permalink / raw)
To: Al Viro
Cc: Jaroslav Kysela, linux-fsdevel, Amadeusz Sławiński,
Takashi Iwai, linux-sound, Vinod Koul
On Thu, 26 Dec 2024 23:17:26 +0100,
Al Viro wrote:
>
> On Thu, Dec 26, 2024 at 09:31:22PM +0000, Al Viro wrote:
> > On Thu, Dec 26, 2024 at 08:00:18PM +0100, Jaroslav Kysela wrote:
> >
> > > I already made almost similar patch:
> > >
> > > https://lore.kernel.org/linux-sound/20241217100726.732863-1-perex@perex.cz/
> >
> > Umm... The only problem with your variant is that dma_buf_get()
> > is wrong here - it should be get_dma_buf() on actual objects,
> > and it should be done before fd_install().
>
> Incremental on top of what just got merged into mainline:
>
> Grab the references to dmabuf before moving them into descriptor
> table - trying to do that by descriptor afterwards might end up getting
> a different object, with a dangling reference left in task->{input,output}
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Could you resubmit this one as a formal patch to be merged?
Thanks!
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c
2024-12-29 8:35 ` Takashi Iwai
@ 2024-12-29 18:53 ` Al Viro
0 siblings, 0 replies; 6+ messages in thread
From: Al Viro @ 2024-12-29 18:53 UTC (permalink / raw)
To: Takashi Iwai
Cc: Jaroslav Kysela, linux-fsdevel, Amadeusz Sławiński,
linux-sound, Vinod Koul
On Sun, Dec 29, 2024 at 09:35:13AM +0100, Takashi Iwai wrote:
> On Thu, 26 Dec 2024 23:17:26 +0100,
> Al Viro wrote:
> >
> > On Thu, Dec 26, 2024 at 09:31:22PM +0000, Al Viro wrote:
> > > On Thu, Dec 26, 2024 at 08:00:18PM +0100, Jaroslav Kysela wrote:
> > >
> > > > I already made almost similar patch:
> > > >
> > > > https://lore.kernel.org/linux-sound/20241217100726.732863-1-perex@perex.cz/
> > >
> > > Umm... The only problem with your variant is that dma_buf_get()
> > > is wrong here - it should be get_dma_buf() on actual objects,
> > > and it should be done before fd_install().
> >
> > Incremental on top of what just got merged into mainline:
> >
> > Grab the references to dmabuf before moving them into descriptor
> > table - trying to do that by descriptor afterwards might end up getting
> > a different object, with a dangling reference left in task->{input,output}
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> Could you resubmit this one as a formal patch to be merged?
> Thanks!
Done (https://lore.kernel.org/all/20241229185232.GA1977892@ZenIV/)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-29 18:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-26 18:29 [CFT][PATCH] fix descriptor uses in sound/core/compress_offload.c Al Viro
2024-12-26 19:00 ` Jaroslav Kysela
2024-12-26 21:31 ` Al Viro
2024-12-26 22:17 ` Al Viro
2024-12-29 8:35 ` Takashi Iwai
2024-12-29 18:53 ` Al Viro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox