[PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[Mathias Krause]

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.
+		 */
 		return -EFAULT;
 	}
 
-- 
2.30.2

Re: [PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[Greg Kroah-Hartman]

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.
> +		 */
>  		return -EFAULT;
>  	}
>  

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.

thanks,

greg k-h

Re: [PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[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.
> > +		 */
> >  		return -EFAULT;
> >  	}
> >  
> 
> 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

Re: [PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[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.
> > +		 */
> >  		return -EFAULT;
> >  	}
> >  
> 
> 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

Re: [PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[Mathias Krause]

Am 27.01.22 um 14:24 schrieb Greg Kroah-Hartman:
> 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.
>> +		 */
>>  		return -EFAULT;
>>  	}
>>  
> 
> This feels wrong.  How do all other dma buf users handle this?

Other dma_buf_fd() users don't wrap the returned fd in another structure
and simply return the fd, i.e. have no error path on the way out of the
kernel.

Thanks,
Mathias

> And you forgot to cc: the dmabuf developers, I think get_maintainers.pl
> should have caught them on this patch.
> 
> thanks,
> 
> greg k-h

Re: [PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[Christian König]

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.
>>> +		 */
>>>   		return -EFAULT;
>>>   	}
>>>   
>> 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

Re: [PATCH] misc: fastrpc: avoid double fput() on failed usercopy

[Christian König]

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.
>>> +		 */
>>>   		return -EFAULT;
>>>   	}
>>>   
>> 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