On Thu, Jan 27, 2022 at 02:24:29PM +0100, Greg Kroah-Hartman wrote:
On Thu, Jan 27, 2022 at 02:02:18PM +0100, Mathias Krause wrote:
If the copy back to userland fails for the FASTRPC_IOCTL_ALLOC_DMA_BUFF ioctl(), we shouldn't assume that 'buf->dmabuf' is still valid. In fact, dma_buf_fd() called fd_install() before, i.e. "consumed" one reference, leaving us with none.
Calling dma_buf_put() will therefore put a reference we no longer own, leading to a valid file descritor table entry for an already released 'file' object which is a straight use-after-free.
Simply avoid calling dma_buf_put() and rely on the process exit code to do the necessary cleanup, if needed, i.e. if the file descriptor is still valid.
Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter") Signed-off-by: Mathias Krause minipli@grsecurity.net
drivers/misc/fastrpc.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 4ccbf43e6bfa..aa1682b94a23 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1288,7 +1288,14 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp) } if (copy_to_user(argp, &bp, sizeof(bp))) {
dma_buf_put(buf->dmabuf);
/** The usercopy failed, but we can't do much about it, as* dma_buf_fd() already called fd_install() and made the* file descriptor accessible for the current process. It* might already be closed and dmabuf no longer valid when* we reach this point. Therefore "leak" the fd and rely on* the process exit path to do any required cleanup.*/This feels wrong. How do all other dma buf users handle this?
And you forgot to cc: the dmabuf developers, I think get_maintainers.pl should have caught them on this patch.
Odd, it didn't, not your fault, my apologies.
DMA BUFFER maintainers, what happened to the MAINTAINERS regex that caused the above patch to not catch you all?
thanks,
greg k-h
Am 27.01.22 um 14:26 schrieb Greg Kroah-Hartman:
On Thu, Jan 27, 2022 at 02:24:29PM +0100, Greg Kroah-Hartman wrote:
On Thu, Jan 27, 2022 at 02:02:18PM +0100, Mathias Krause wrote:
If the copy back to userland fails for the FASTRPC_IOCTL_ALLOC_DMA_BUFF ioctl(), we shouldn't assume that 'buf->dmabuf' is still valid. In fact, dma_buf_fd() called fd_install() before, i.e. "consumed" one reference, leaving us with none.
Calling dma_buf_put() will therefore put a reference we no longer own, leading to a valid file descritor table entry for an already released 'file' object which is a straight use-after-free.
Simply avoid calling dma_buf_put() and rely on the process exit code to do the necessary cleanup, if needed, i.e. if the file descriptor is still valid.
Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter") Signed-off-by: Mathias Krause minipli@grsecurity.net
drivers/misc/fastrpc.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 4ccbf43e6bfa..aa1682b94a23 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1288,7 +1288,14 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp) } if (copy_to_user(argp, &bp, sizeof(bp))) {
dma_buf_put(buf->dmabuf);
/** The usercopy failed, but we can't do much about it, as* dma_buf_fd() already called fd_install() and made the* file descriptor accessible for the current process. It* might already be closed and dmabuf no longer valid when* we reach this point. Therefore "leak" the fd and rely on* the process exit path to do any required cleanup.*/This feels wrong. How do all other dma buf users handle this?
And you forgot to cc: the dmabuf developers, I think get_maintainers.pl should have caught them on this patch.
Odd, it didn't, not your fault, my apologies.
DMA BUFFER maintainers, what happened to the MAINTAINERS regex that caused the above patch to not catch you all?
That worked as expected: \bdma_(?:buf|fence|resv)\b
We are only automatically getting CCed when any of the dma_buf, dma_fence or dma_resv structures are mentioned.
We could remove the trailing \b and match the function names as well.
Regards, Christian.
thanks,
greg k-h
linaro-mm-sig@lists.linaro.org
-
Christian König -
Greg Kroah-Hartman