| Message ID | 20241226182959.GU1977892@ZenIV (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [CFT] fix descriptor uses in sound/core/compress_offload.c | expand |
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
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().
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;
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;
[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> ---