[PATCH v1 0/2] Add missing fixes in fastrpc_get_args

[PATCH v1 0/2] Add missing fixes in fastrpc_get_args

[Ekansh Gupta]

This patch series adds the listed bug fixes that have been missing
in upstream fastRPC driver:
- Page address for registered buffer(with fd) is not calculated
  properly.
- Page size calculation for non-registered buffer(copy buffer) is
  incorrect.

Ekansh Gupta (2):
  misc: fastrpc: Fix registered buffer page address
  misc: fastrpc: Fix copy buffer page size

 drivers/misc/fastrpc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] misc: fastrpc: Fix registered buffer page address

[Ekansh Gupta]

For registered  buffers, fastrpc driver sends the buffer information
to remote subsystem. There is a problem with current implementation
where the page address is being sent with an offset leading to
improper buffer address on DSP. This is leads to functional failures
as DSP expects base address in page information and extracts offset
information from remote arguments. Mask the offset and pass the base
page address to DSP.

Fixes: 80f3afd72bd4 ("misc: fastrpc: consider address offset before sending to DSP")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 48d08eeb2d20..cfa1546c9e3f 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -992,7 +992,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			mmap_read_lock(current->mm);
 			vma = find_vma(current->mm, ctx->args[i].ptr);
 			if (vma)
-				pages[i].addr += ctx->args[i].ptr -
+				pages[i].addr += (ctx->args[i].ptr & PAGE_MASK) -
 						 vma->vm_start;
 			mmap_read_unlock(current->mm);
 
-- 
2.34.1

Re: [PATCH v1 1/2] misc: fastrpc: Fix registered buffer page address

[Dmitry Baryshkov]

On Wed, Dec 18, 2024 at 03:54:28PM +0530, Ekansh Gupta wrote:
> For registered  buffers, fastrpc driver sends the buffer information
> to remote subsystem. There is a problem with current implementation
> where the page address is being sent with an offset leading to
> improper buffer address on DSP. This is leads to functional failures
> as DSP expects base address in page information and extracts offset
> information from remote arguments. Mask the offset and pass the base
> page address to DSP.
> 
> Fixes: 80f3afd72bd4 ("misc: fastrpc: consider address offset before sending to DSP")

This was committed in 2019. Are you saying that the driver has been
broken since that time? If so, what is the impact? Because I've
definitely been running fastrpc workload after that moment.

Also, is there any reason for neglecting checkpatch warning?

> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 48d08eeb2d20..cfa1546c9e3f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -992,7 +992,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>  			mmap_read_lock(current->mm);
>  			vma = find_vma(current->mm, ctx->args[i].ptr);
>  			if (vma)
> -				pages[i].addr += ctx->args[i].ptr -
> +				pages[i].addr += (ctx->args[i].ptr & PAGE_MASK) -
>  						 vma->vm_start;
>  			mmap_read_unlock(current->mm);
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/2] misc: fastrpc: Fix registered buffer page address

[Ekansh Gupta]

On 12/18/2024 4:42 PM, Dmitry Baryshkov wrote:
> On Wed, Dec 18, 2024 at 03:54:28PM +0530, Ekansh Gupta wrote:
>> For registered  buffers, fastrpc driver sends the buffer information
>> to remote subsystem. There is a problem with current implementation
>> where the page address is being sent with an offset leading to
>> improper buffer address on DSP. This is leads to functional failures
>> as DSP expects base address in page information and extracts offset
>> information from remote arguments. Mask the offset and pass the base
>> page address to DSP.
>>
>> Fixes: 80f3afd72bd4 ("misc: fastrpc: consider address offset before sending to DSP")
> This was committed in 2019. Are you saying that the driver has been
> broken since that time? If so, what is the impact? Because I've
> definitely been running fastrpc workload after that moment.
>
> Also, is there any reason for neglecting checkpatch warning?
Hi Dmitry,

This issue is observed is a corner case when some buffer which is registered with fastrpc
framework is passed with some offset by user and then the DSP implementation tried to
read the data. As DSP expects base address and takes care of offsetting with remote
arguments, passing an offsetted address will result in some unexpected data read in DSP.

All generic usecases usually pass the buffer as it is hence is problem is not usually observed. If
someone tries to pass offsetted buffer and then tries to compare data at HLOS and DSP end,
then the ambiguity will be observed.

Apologies for delay in response as I was traveling with very limited internet access.

--ekansh
>
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 48d08eeb2d20..cfa1546c9e3f 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -992,7 +992,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>>  			mmap_read_lock(current->mm);
>>  			vma = find_vma(current->mm, ctx->args[i].ptr);
>>  			if (vma)
>> -				pages[i].addr += ctx->args[i].ptr -
>> +				pages[i].addr += (ctx->args[i].ptr & PAGE_MASK) -
>>  						 vma->vm_start;
>>  			mmap_read_unlock(current->mm);
>>  
>> -- 
>> 2.34.1
>>

Re: [PATCH v1 1/2] misc: fastrpc: Fix registered buffer page address

[Dmitry Baryshkov]

On Thu, Jan 09, 2025 at 11:09:30AM +0530, Ekansh Gupta wrote:
> 
> 
> 
> On 12/18/2024 4:42 PM, Dmitry Baryshkov wrote:
> > On Wed, Dec 18, 2024 at 03:54:28PM +0530, Ekansh Gupta wrote:
> >> For registered  buffers, fastrpc driver sends the buffer information
> >> to remote subsystem. There is a problem with current implementation
> >> where the page address is being sent with an offset leading to
> >> improper buffer address on DSP. This is leads to functional failures
> >> as DSP expects base address in page information and extracts offset
> >> information from remote arguments. Mask the offset and pass the base
> >> page address to DSP.
> >>
> >> Fixes: 80f3afd72bd4 ("misc: fastrpc: consider address offset before sending to DSP")
> > This was committed in 2019. Are you saying that the driver has been
> > broken since that time? If so, what is the impact? Because I've
> > definitely been running fastrpc workload after that moment.
> >
> > Also, is there any reason for neglecting checkpatch warning?
> Hi Dmitry,
> 
> This issue is observed is a corner case when some buffer which is registered with fastrpc
> framework is passed with some offset by user and then the DSP implementation tried to
> read the data. As DSP expects base address and takes care of offsetting with remote
> arguments, passing an offsetted address will result in some unexpected data read in DSP.
> 
> All generic usecases usually pass the buffer as it is hence is problem is not usually observed. If
> someone tries to pass offsetted buffer and then tries to compare data at HLOS and DSP end,
> then the ambiguity will be observed.

Ok. Thanks for the explanation. Please consider moving relevant bits to
the commit message.

Also this brings up a topic that we have discussed several times: what
is the progress on a testsuite for the API?

Last, but not least, does this issue result in a possible access to
unrelated memory areas? Can it be exploited somehow?

> 
> Apologies for delay in response as I was traveling with very limited internet access.
> 
> --ekansh
> >
> >> Cc: stable <stable@kernel.org>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index 48d08eeb2d20..cfa1546c9e3f 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -992,7 +992,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
> >>  			mmap_read_lock(current->mm);
> >>  			vma = find_vma(current->mm, ctx->args[i].ptr);
> >>  			if (vma)
> >> -				pages[i].addr += ctx->args[i].ptr -
> >> +				pages[i].addr += (ctx->args[i].ptr & PAGE_MASK) -
> >>  						 vma->vm_start;

Shouldn't it be other way around:

	pages[i].addr += (ctx->args[i].ptr - vma->vm_start) & PAGE_MASK;


Also, can offset be larger than a page size?

> >>  			mmap_read_unlock(current->mm);
> >>  
> >> -- 
> >> 2.34.1
> >>
> 

-- 
With best wishes
Dmitry

[PATCH v1 2/2] misc: fastrpc: Fix copy buffer page size

[Ekansh Gupta]

For non-registered buffer, fastrpc driver copies the buffer and
pass it to the remote subsystem. There is a problem with current
implementation of page size calculation which is not considering
the offset in the calculation. This might lead to passing of
improper and out-of-bounds page size which could result in
memory issue. Calculate page start and page end using the offset
adjusted address instead of absolute address.

Fixes: 02b45b47fbe8 ("misc: fastrpc: fix remote page size calculation")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index cfa1546c9e3f..00154c888c45 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1019,8 +1019,8 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 					(pkt_size - rlen);
 			pages[i].addr = pages[i].addr &	PAGE_MASK;
 
-			pg_start = (args & PAGE_MASK) >> PAGE_SHIFT;
-			pg_end = ((args + len - 1) & PAGE_MASK) >> PAGE_SHIFT;
+			pg_start = (rpra[i].buf.pv & PAGE_MASK) >> PAGE_SHIFT;
+			pg_end = ((rpra[i].buf.pv + len - 1) & PAGE_MASK) >> PAGE_SHIFT;
 			pages[i].size = (pg_end - pg_start + 1) * PAGE_SIZE;
 			args = args + mlen;
 			rlen -= mlen;
-- 
2.34.1

Re: [PATCH v1 2/2] misc: fastrpc: Fix copy buffer page size

[Dmitry Baryshkov]

On Wed, Dec 18, 2024 at 03:54:29PM +0530, Ekansh Gupta wrote:
> For non-registered buffer, fastrpc driver copies the buffer and
> pass it to the remote subsystem. There is a problem with current
> implementation of page size calculation which is not considering
> the offset in the calculation. This might lead to passing of
> improper and out-of-bounds page size which could result in
> memory issue. Calculate page start and page end using the offset
> adjusted address instead of absolute address.

Which offset?

> 
> Fixes: 02b45b47fbe8 ("misc: fastrpc: fix remote page size calculation")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index cfa1546c9e3f..00154c888c45 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1019,8 +1019,8 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>  					(pkt_size - rlen);
>  			pages[i].addr = pages[i].addr &	PAGE_MASK;
>  
> -			pg_start = (args & PAGE_MASK) >> PAGE_SHIFT;
> -			pg_end = ((args + len - 1) & PAGE_MASK) >> PAGE_SHIFT;
> +			pg_start = (rpra[i].buf.pv & PAGE_MASK) >> PAGE_SHIFT;
> +			pg_end = ((rpra[i].buf.pv + len - 1) & PAGE_MASK) >> PAGE_SHIFT;
>  			pages[i].size = (pg_end - pg_start + 1) * PAGE_SIZE;
>  			args = args + mlen;
>  			rlen -= mlen;
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 0/2] Add missing fixes in fastrpc_get_args

[Dmitry Baryshkov]

On Wed, Dec 18, 2024 at 03:54:27PM +0530, Ekansh Gupta wrote:
> This patch series adds the listed bug fixes that have been missing
> in upstream fastRPC driver:
> - Page address for registered buffer(with fd) is not calculated
>   properly.
> - Page size calculation for non-registered buffer(copy buffer) is
>   incorrect.

Please describe something non-obvious here. You are basically repeating
the list of patches that comes in the next line.

> 
> Ekansh Gupta (2):
>   misc: fastrpc: Fix registered buffer page address
>   misc: fastrpc: Fix copy buffer page size
> 
>  drivers/misc/fastrpc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 0/2] Add missing fixes in fastrpc_get_args

[Srinivas Kandagatla]

On Wed, 18 Dec 2024 15:54:27 +0530, Ekansh Gupta wrote:
> This patch series adds the listed bug fixes that have been missing
> in upstream fastRPC driver:
> - Page address for registered buffer(with fd) is not calculated
>   properly.
> - Page size calculation for non-registered buffer(copy buffer) is
>   incorrect.
> 
> [...]

Applied, thanks!

[1/2] misc: fastrpc: Fix registered buffer page address
      commit: fa22a9743aece593fe9f1e0a0d6189a777d67e38
[2/2] misc: fastrpc: Fix copy buffer page size
      commit: 58570026c7ac249bfbd90f9fcb7d2e0a74a106a1

Best regards,
-- 
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Re: [PATCH v1 0/2] Add missing fixes in fastrpc_get_args

[Dmitry Baryshkov]

On Mon, Dec 30, 2024 at 04:15:42PM +0000, Srinivas Kandagatla wrote:
> 
> On Wed, 18 Dec 2024 15:54:27 +0530, Ekansh Gupta wrote:
> > This patch series adds the listed bug fixes that have been missing
> > in upstream fastRPC driver:
> > - Page address for registered buffer(with fd) is not calculated
> >   properly.
> > - Page size calculation for non-registered buffer(copy buffer) is
> >   incorrect.
> > 
> > [...]
> 
> Applied, thanks!

May I ask, why they are being accepted with the obvious checkpatch
warnings?

What kind of process is being followed, as those patches had review
comments to be implemented in the next iteration.

> 
> [1/2] misc: fastrpc: Fix registered buffer page address
>       commit: fa22a9743aece593fe9f1e0a0d6189a777d67e38
> [2/2] misc: fastrpc: Fix copy buffer page size
>       commit: 58570026c7ac249bfbd90f9fcb7d2e0a74a106a1
> 
> Best regards,
> -- 
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 0/2] Add missing fixes in fastrpc_get_args

[Srinivas Kandagatla]

On 30/12/2024 18:22, Dmitry Baryshkov wrote:
> On Mon, Dec 30, 2024 at 04:15:42PM +0000, Srinivas Kandagatla wrote:
>>
>> On Wed, 18 Dec 2024 15:54:27 +0530, Ekansh Gupta wrote:
>>> This patch series adds the listed bug fixes that have been missing
>>> in upstream fastRPC driver:
>>> - Page address for registered buffer(with fd) is not calculated
>>>    properly.
>>> - Page size calculation for non-registered buffer(copy buffer) is
>>>    incorrect.
>>>
>>> [...]
>>
>> Applied, thanks!
> 
> May I ask, why they are being accepted with the obvious checkpatch
> warnings?

If you are referring to this warning.
WARNING: Invalid email format for stable: 'stable <stable@kernel.org>', 
prefer 'stable@kernel.org'

I tend to fix such small warnings before applying. These are fixed now.

> 
> What kind of process is being followed, as those patches had review
> comments to be implemented in the next iteration.

I apply these patches if it looks good to me. This also helps with 
getting it tested from wider audience via linux-next.

I do run TFLite workloads before it ends up in char-misc, but not for 
every patch.

sorry If I missed any blocker comments, but your comments were more on 
the cover letter content and asking about the work loads which triggers 
these bugs.

Are these patches breaking any of your test-cases?


--srini

Re: [PATCH v1 0/2] Add missing fixes in fastrpc_get_args

[Dmitry Baryshkov]

On Mon, Dec 30, 2024 at 08:32:30PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 30/12/2024 18:22, Dmitry Baryshkov wrote:
> > On Mon, Dec 30, 2024 at 04:15:42PM +0000, Srinivas Kandagatla wrote:
> > > 
> > > On Wed, 18 Dec 2024 15:54:27 +0530, Ekansh Gupta wrote:
> > > > This patch series adds the listed bug fixes that have been missing
> > > > in upstream fastRPC driver:
> > > > - Page address for registered buffer(with fd) is not calculated
> > > >    properly.
> > > > - Page size calculation for non-registered buffer(copy buffer) is
> > > >    incorrect.
> > > > 
> > > > [...]
> > > 
> > > Applied, thanks!
> > 
> > May I ask, why they are being accepted with the obvious checkpatch
> > warnings?
> 
> If you are referring to this warning.
> WARNING: Invalid email format for stable: 'stable <stable@kernel.org>',
> prefer 'stable@kernel.org'
> 
> I tend to fix such small warnings before applying. These are fixed now.
> 
> > 
> > What kind of process is being followed, as those patches had review
> > comments to be implemented in the next iteration.
> 
> I apply these patches if it looks good to me. This also helps with getting
> it tested from wider audience via linux-next.
> 
> I do run TFLite workloads before it ends up in char-misc, but not for every
> patch.
> 
> sorry If I missed any blocker comments, but your comments were more on the
> cover letter content and asking about the work loads which triggers these
> bugs.
> 
> Are these patches breaking any of your test-cases?

No. But info about work-loads is the most important part: it makes sure
that none of the developers miss similar issue next time.

-- 
With best wishes
Dmitry