[PATCH] misc: fastrpc: fix improper packet size calculation

[PATCH] misc: fastrpc: fix improper packet size calculation

[Jeya R]

The buffer list is sorted and this is not being
considered while calculating packet size. This
would lead to improper copy length calculation
for non-dmaheap buffers which would eventually
cause sending improper buffers to DSP.

Signed-off-by: Jeya R <jeyr@codeaurora.org>
---
 drivers/misc/fastrpc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index beda610..a7e550f 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -719,16 +719,21 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx)
 static u64 fastrpc_get_payload_size(struct fastrpc_invoke_ctx *ctx, int metalen)
 {
 	u64 size = 0;
-	int i;
+	int oix = 0;
 
 	size = ALIGN(metalen, FASTRPC_ALIGN);
-	for (i = 0; i < ctx->nscalars; i++) {
+	for (oix = 0; oix < ctx->nbufs; oix++) {
+		int i = ctx->olaps[oix].raix;
+
+		if (ctx->args[i].length == 0)
+			continue;
+
 		if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1) {
 
-			if (ctx->olaps[i].offset == 0)
+			if (ctx->olaps[oix].offset == 0)
 				size = ALIGN(size, FASTRPC_ALIGN);
 
-			size += (ctx->olaps[i].mend - ctx->olaps[i].mstart);
+			size += (ctx->olaps[oix].mend - ctx->olaps[oix].mstart);
 		}
 	}
 
-- 
2.7.4

Re: [PATCH] misc: fastrpc: fix improper packet size calculation

[Greg KH]

On Mon, Sep 20, 2021 at 01:45:31PM +0530, Jeya R wrote:
> The buffer list is sorted and this is not being
> considered while calculating packet size. This
> would lead to improper copy length calculation
> for non-dmaheap buffers which would eventually
> cause sending improper buffers to DSP.

You do have the full 72 columns to use :)

> 
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> ---
>  drivers/misc/fastrpc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

What commit does this fix?

thanks,

greg k-h

Re: [PATCH] misc: fastrpc: fix improper packet size calculation

[jeyr]

On 2021-09-20 14:38, Greg KH wrote:
> On Mon, Sep 20, 2021 at 01:45:31PM +0530, Jeya R wrote:
>> The buffer list is sorted and this is not being
>> considered while calculating packet size. This
>> would lead to improper copy length calculation
>> for non-dmaheap buffers which would eventually
>> cause sending improper buffers to DSP.
> 
> You do have the full 72 columns to use :)

Thanks, will update the commit message considering this.

> 
>> 
>> Signed-off-by: Jeya R <jeyr@codeaurora.org>
>> ---
>>  drivers/misc/fastrpc.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> What commit does this fix?
> 
> thanks,
> 
> greg k-h

Payload calculation function was modified to handle buffer overlapping 
calculation in this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/misc/fastrpc.c?h=v5.15-rc2&id=25e8dfb83cda0a123bb1e091d6c3599cde050d76

Here during buffer overlap calculation, the buffer list is getting 
sorted. This needs to be considered during the calculation of payload 
size also by using unsorted buffer index "raix".

Re: [PATCH] misc: fastrpc: fix improper packet size calculation

[Greg KH]

On Mon, Sep 20, 2021 at 03:01:07PM +0530, jeyr@codeaurora.org wrote:
> On 2021-09-20 14:38, Greg KH wrote:
> > On Mon, Sep 20, 2021 at 01:45:31PM +0530, Jeya R wrote:
> > > The buffer list is sorted and this is not being
> > > considered while calculating packet size. This
> > > would lead to improper copy length calculation
> > > for non-dmaheap buffers which would eventually
> > > cause sending improper buffers to DSP.
> > 
> > You do have the full 72 columns to use :)
> 
> Thanks, will update the commit message considering this.
> 
> > 
> > > 
> > > Signed-off-by: Jeya R <jeyr@codeaurora.org>
> > > ---
> > >  drivers/misc/fastrpc.c | 13 +++++++++----
> > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > What commit does this fix?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Payload calculation function was modified to handle buffer overlapping
> calculation in this commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/misc/fastrpc.c?h=v5.15-rc2&id=25e8dfb83cda0a123bb1e091d6c3599cde050d76
> 
> Here during buffer overlap calculation, the buffer list is getting sorted.
> This needs to be considered during the calculation of payload size also by
> using unsorted buffer index "raix".

Ok, then please add a "Fixes:" tag when you resend this.

thanks,

greg k-h

Re: [PATCH] misc: fastrpc: fix improper packet size calculation

[Srinivas Kandagatla]

Thanks Jeya for the fix,

On 20/09/2021 09:15, Jeya R wrote:
> The buffer list is sorted and this is not being
> considered while calculating packet size. This
> would lead to improper copy length calculation
> for non-dmaheap buffers which would eventually
> cause sending improper buffers to DSP.
> 
we need below fixes tag:

Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")

> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> ---
>   drivers/misc/fastrpc.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index beda610..a7e550f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -719,16 +719,21 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx)
>   static u64 fastrpc_get_payload_size(struct fastrpc_invoke_ctx *ctx, int metalen)
>   {
>   	u64 size = 0;
> -	int i;
> +	int oix = 0;
Unnecessary initialization here.


>   
>   	size = ALIGN(metalen, FASTRPC_ALIGN);
> -	for (i = 0; i < ctx->nscalars; i++) {
> +	for (oix = 0; oix < ctx->nbufs; oix++) {
> +		int i = ctx->olaps[oix].raix;
> +

[--
> +		if (ctx->args[i].length == 0)
> +			continue;
> +
--]
This additional check will not really have any effect on size 
calculations, as (ctx->olaps[oix].mend - ctx->olaps[oix].mstart) would 
result in zero.
Any reason why this check was added?

--srini


>   		if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1) {
>   
> -			if (ctx->olaps[i].offset == 0)
> +			if (ctx->olaps[oix].offset == 0)
>   				size = ALIGN(size, FASTRPC_ALIGN);
>   
> -			size += (ctx->olaps[i].mend - ctx->olaps[i].mstart);
> +			size += (ctx->olaps[oix].mend - ctx->olaps[oix].mstart);
>   		}
>   	}
>   
> 

Re: [PATCH] misc: fastrpc: fix improper packet size calculation

[Srinivas Kandagatla]

Hi Jeya,

Can you make sure the subject line reflects the patch version that you 
are sending.
In this case it should be "[PATCH v2] misc: fastrpc: fix improper packet 
size calculation"

this will help reviewers and maintainers to differentiate the versions 
of patch.

On 21/09/2021 11:35, Jeya R wrote:
> The buffer list is sorted and this is not being considered while
> calculating packet size. This would lead to improper copy length
> calculation for non-dmaheap buffers which would eventually cause
> sending improper buffers to DSP.
> 
> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> ---

Please add changes done from v1 to v2 here.

something like:

Changes from v1:
- bla bla bla..


>   drivers/misc/fastrpc.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index beda610..a7e550f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -719,16 +719,21 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx)
>   static u64 fastrpc_get_payload_size(struct fastrpc_invoke_ctx *ctx, int metalen)
>   {
>   	u64 size = 0;
> -	int i;
> +	int oix = 0;

Looks like you missed to address my previous comments.

--srini
>   
>   	size = ALIGN(metalen, FASTRPC_ALIGN);
> -	for (i = 0; i < ctx->nscalars; i++) {
> +	for (oix = 0; oix < ctx->nbufs; oix++) {
> +		int i = ctx->olaps[oix].raix;
> +
> +		if (ctx->args[i].length == 0)
> +			continue;
> +
>   		if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1) {
>   
> -			if (ctx->olaps[i].offset == 0)
> +			if (ctx->olaps[oix].offset == 0)
>   				size = ALIGN(size, FASTRPC_ALIGN);
>   
> -			size += (ctx->olaps[i].mend - ctx->olaps[i].mstart);
> +			size += (ctx->olaps[oix].mend - ctx->olaps[oix].mstart);
>   		}
>   	}
>   
> 

[PATCH] misc: fastrpc: fix improper packet size calculation

[Jeya R]

The buffer list is sorted and this is not being considered while
calculating packet size. This would lead to improper copy length
calculation for non-dmaheap buffers which would eventually cause
sending improper buffers to DSP.

Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
Signed-off-by: Jeya R <jeyr@codeaurora.org>
---
 drivers/misc/fastrpc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index beda610..a7e550f 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -719,16 +719,21 @@ static int fastrpc_get_meta_size(struct fastrpc_invoke_ctx *ctx)
 static u64 fastrpc_get_payload_size(struct fastrpc_invoke_ctx *ctx, int metalen)
 {
 	u64 size = 0;
-	int i;
+	int oix = 0;
 
 	size = ALIGN(metalen, FASTRPC_ALIGN);
-	for (i = 0; i < ctx->nscalars; i++) {
+	for (oix = 0; oix < ctx->nbufs; oix++) {
+		int i = ctx->olaps[oix].raix;
+
+		if (ctx->args[i].length == 0)
+			continue;
+
 		if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1) {
 
-			if (ctx->olaps[i].offset == 0)
+			if (ctx->olaps[oix].offset == 0)
 				size = ALIGN(size, FASTRPC_ALIGN);
 
-			size += (ctx->olaps[i].mend - ctx->olaps[i].mstart);
+			size += (ctx->olaps[oix].mend - ctx->olaps[oix].mstart);
 		}
 	}
 
-- 
2.7.4