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

[PATCH v3] 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>
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Changes in v3:
- relocate patch change list

Changes in v2:
- updated commit message to proper format
- added fixes tag to commit message
- removed unnecessary variable initialization
- removed length check during payload calculation

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

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index beda610..69d45c4 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -719,16 +719,18 @@ 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;
 
 	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].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 v3] misc: fastrpc: fix improper packet size calculation

[Greg KH]

On Tue, Sep 21, 2021 at 05:18:15PM +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.
> 
> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
> Signed-off-by: Jeya R <jeyr@codeaurora.org>
> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Does this also need to go to the stable kernels?

> ---
> Changes in v3:
> - relocate patch change list
> 
> Changes in v2:
> - updated commit message to proper format
> - added fixes tag to commit message
> - removed unnecessary variable initialization
> - removed length check during payload calculation
> 
>  drivers/misc/fastrpc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index beda610..69d45c4 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -719,16 +719,18 @@ 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;

What does "oix" stand for?  What was wrong with i?

thanks,

greg k-h

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

[jeyr]

On 2021-09-21 17:22, Greg KH wrote:
> On Tue, Sep 21, 2021 at 05:18:15PM +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.
>> 
>> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke 
>> method")
>> Signed-off-by: Jeya R <jeyr@codeaurora.org>
>> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> Does this also need to go to the stable kernels?
Yes, this needs to go to stable kernels also as this fixes a potential 
issue which is easily reproducible.

> 
>> ---
>> Changes in v3:
>> - relocate patch change list
>> 
>> Changes in v2:
>> - updated commit message to proper format
>> - added fixes tag to commit message
>> - removed unnecessary variable initialization
>> - removed length check during payload calculation
>> 
>>  drivers/misc/fastrpc.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index beda610..69d45c4 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -719,16 +719,18 @@ 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;
> 
> What does "oix" stand for?  What was wrong with i?
It is just a general convention we use. "oix" is used to iterate through 
sorted overlap buffer list and use "i" to get corresponding unsorted 
list index. We follow the same convention at other places also, for 
example: fastrpc_get_args function.
> 
> thanks,
> 
> greg k-h

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

[Greg KH]

On Tue, Sep 21, 2021 at 06:03:42PM +0530, jeyr@codeaurora.org wrote:
> On 2021-09-21 17:22, Greg KH wrote:
> > On Tue, Sep 21, 2021 at 05:18:15PM +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.
> > > 
> > > Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke
> > > method")
> > > Signed-off-by: Jeya R <jeyr@codeaurora.org>
> > > Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > 
> > Does this also need to go to the stable kernels?
> Yes, this needs to go to stable kernels also as this fixes a potential issue
> which is easily reproducible.



> 
> > 
> > > ---
> > > Changes in v3:
> > > - relocate patch change list
> > > 
> > > Changes in v2:
> > > - updated commit message to proper format
> > > - added fixes tag to commit message
> > > - removed unnecessary variable initialization
> > > - removed length check during payload calculation
> > > 
> > >  drivers/misc/fastrpc.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > index beda610..69d45c4 100644
> > > --- a/drivers/misc/fastrpc.c
> > > +++ b/drivers/misc/fastrpc.c
> > > @@ -719,16 +719,18 @@ 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;
> > 
> > What does "oix" stand for?  What was wrong with i?
> It is just a general convention we use. "oix" is used to iterate through
> sorted overlap buffer list and use "i" to get corresponding unsorted list
> index. We follow the same convention at other places also, for example:
> fastrpc_get_args function.

That is the only place it is used in all of the whole kernel tree.  It
is not a normal variable for a loop, so who is "we" here?

thanks,

greg k-h

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

[jeyr]

On 2021-09-21 18:10, Greg KH wrote:
> On Tue, Sep 21, 2021 at 06:03:42PM +0530, jeyr@codeaurora.org wrote:
>> On 2021-09-21 17:22, Greg KH wrote:
>> > On Tue, Sep 21, 2021 at 05:18:15PM +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.
>> > >
>> > > Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke
>> > > method")
>> > > Signed-off-by: Jeya R <jeyr@codeaurora.org>
>> > > Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> >
>> > Does this also need to go to the stable kernels?
>> Yes, this needs to go to stable kernels also as this fixes a potential 
>> issue
>> which is easily reproducible.
> 
> 
> 
>> 
>> >
>> > > ---
>> > > Changes in v3:
>> > > - relocate patch change list
>> > >
>> > > Changes in v2:
>> > > - updated commit message to proper format
>> > > - added fixes tag to commit message
>> > > - removed unnecessary variable initialization
>> > > - removed length check during payload calculation
>> > >
>> > >  drivers/misc/fastrpc.c | 10 ++++++----
>> > >  1 file changed, 6 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> > > index beda610..69d45c4 100644
>> > > --- a/drivers/misc/fastrpc.c
>> > > +++ b/drivers/misc/fastrpc.c
>> > > @@ -719,16 +719,18 @@ 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;
>> >
>> > What does "oix" stand for?  What was wrong with i?
>> It is just a general convention we use. "oix" is used to iterate 
>> through
>> sorted overlap buffer list and use "i" to get corresponding unsorted 
>> list
>> index. We follow the same convention at other places also, for 
>> example:
>> fastrpc_get_args function.
> 
> That is the only place it is used in all of the whole kernel tree.  It
> is not a normal variable for a loop, so who is "we" here?
The convention was followed for the same file(fastrpc.c). As part of 
fastrpc_get_args
function, while iterating through sorted buffer list, oix is used as 
index and to
get unsorted index "raix", it is using "i". Just following the same way 
here to
have better understanding. Please let me know if this is a concern, it 
can be updated
to "i", "j" etc.

-- Thanks
> 
> thanks,
> 
> greg k-h

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

[jeyr]

On 2021-09-21 18:43, jeyr@codeaurora.org wrote:
> On 2021-09-21 18:10, Greg KH wrote:
>> On Tue, Sep 21, 2021 at 06:03:42PM +0530, jeyr@codeaurora.org wrote:
>>> On 2021-09-21 17:22, Greg KH wrote:
>>> > On Tue, Sep 21, 2021 at 05:18:15PM +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.
>>> > >
>>> > > Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke
>>> > > method")
>>> > > Signed-off-by: Jeya R <jeyr@codeaurora.org>
>>> > > Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> >
>>> > Does this also need to go to the stable kernels?
>>> Yes, this needs to go to stable kernels also as this fixes a 
>>> potential issue
>>> which is easily reproducible.
>> 
>> 
>> 
>>> 
>>> >
>>> > > ---
>>> > > Changes in v3:
>>> > > - relocate patch change list
>>> > >
>>> > > Changes in v2:
>>> > > - updated commit message to proper format
>>> > > - added fixes tag to commit message
>>> > > - removed unnecessary variable initialization
>>> > > - removed length check during payload calculation
>>> > >
>>> > >  drivers/misc/fastrpc.c | 10 ++++++----
>>> > >  1 file changed, 6 insertions(+), 4 deletions(-)
>>> > >
>>> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> > > index beda610..69d45c4 100644
>>> > > --- a/drivers/misc/fastrpc.c
>>> > > +++ b/drivers/misc/fastrpc.c
>>> > > @@ -719,16 +719,18 @@ 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;
>>> >
>>> > What does "oix" stand for?  What was wrong with i?
>>> It is just a general convention we use. "oix" is used to iterate 
>>> through
>>> sorted overlap buffer list and use "i" to get corresponding unsorted 
>>> list
>>> index. We follow the same convention at other places also, for 
>>> example:
>>> fastrpc_get_args function.
>> 
>> That is the only place it is used in all of the whole kernel tree.  It
>> is not a normal variable for a loop, so who is "we" here?
> The convention was followed for the same file(fastrpc.c). As part of
> fastrpc_get_args
> function, while iterating through sorted buffer list, oix is used as
> index and to
> get unsorted index "raix", it is using "i". Just following the same way 
> here to
> have better understanding. Please let me know if this is a concern, it
> can be updated
> to "i", "j" etc.
> 
> -- Thanks
>> 
>> thanks,
>> 
>> greg k-h
Hello Greg,

Is this bug-fix patch planned to be released?

-- Thanks

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

[Greg KH]

On Fri, Nov 19, 2021 at 06:19:27PM +0530, jeyr@codeaurora.org wrote:
> On 2021-09-21 18:43, jeyr@codeaurora.org wrote:
> > On 2021-09-21 18:10, Greg KH wrote:
> > > On Tue, Sep 21, 2021 at 06:03:42PM +0530, jeyr@codeaurora.org wrote:
> > > > On 2021-09-21 17:22, Greg KH wrote:
> > > > > On Tue, Sep 21, 2021 at 05:18:15PM +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.
> > > > > >
> > > > > > Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke
> > > > > > method")
> > > > > > Signed-off-by: Jeya R <jeyr@codeaurora.org>
> > > > > > Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > > >
> > > > > Does this also need to go to the stable kernels?
> > > > Yes, this needs to go to stable kernels also as this fixes a
> > > > potential issue
> > > > which is easily reproducible.
> > > 
> > > 
> > > 
> > > > 
> > > > >
> > > > > > ---
> > > > > > Changes in v3:
> > > > > > - relocate patch change list
> > > > > >
> > > > > > Changes in v2:
> > > > > > - updated commit message to proper format
> > > > > > - added fixes tag to commit message
> > > > > > - removed unnecessary variable initialization
> > > > > > - removed length check during payload calculation
> > > > > >
> > > > > >  drivers/misc/fastrpc.c | 10 ++++++----
> > > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > > > > index beda610..69d45c4 100644
> > > > > > --- a/drivers/misc/fastrpc.c
> > > > > > +++ b/drivers/misc/fastrpc.c
> > > > > > @@ -719,16 +719,18 @@ 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;
> > > > >
> > > > > What does "oix" stand for?  What was wrong with i?
> > > > It is just a general convention we use. "oix" is used to iterate
> > > > through
> > > > sorted overlap buffer list and use "i" to get corresponding
> > > > unsorted list
> > > > index. We follow the same convention at other places also, for
> > > > example:
> > > > fastrpc_get_args function.
> > > 
> > > That is the only place it is used in all of the whole kernel tree.  It
> > > is not a normal variable for a loop, so who is "we" here?
> > The convention was followed for the same file(fastrpc.c). As part of
> > fastrpc_get_args
> > function, while iterating through sorted buffer list, oix is used as
> > index and to
> > get unsorted index "raix", it is using "i". Just following the same way
> > here to
> > have better understanding. Please let me know if this is a concern, it
> > can be updated
> > to "i", "j" etc.
> > 
> > -- Thanks
> > > 
> > > thanks,
> > > 
> > > greg k-h
> Hello Greg,
> 
> Is this bug-fix patch planned to be released?

Released in what way?

I do not see it in any tree anywhere, perhaps it needs to be resubmitted
to be accepted?

thanks,

greg k-h

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

[jeyr]

On 2021-11-19 18:23, Greg KH wrote:
> On Fri, Nov 19, 2021 at 06:19:27PM +0530, jeyr@codeaurora.org wrote:
>> On 2021-09-21 18:43, jeyr@codeaurora.org wrote:
>> > On 2021-09-21 18:10, Greg KH wrote:
>> > > On Tue, Sep 21, 2021 at 06:03:42PM +0530, jeyr@codeaurora.org wrote:
>> > > > On 2021-09-21 17:22, Greg KH wrote:
>> > > > > On Tue, Sep 21, 2021 at 05:18:15PM +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.
>> > > > > >
>> > > > > > Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke
>> > > > > > method")
>> > > > > > Signed-off-by: Jeya R <jeyr@codeaurora.org>
>> > > > > > Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> > > > >
>> > > > > Does this also need to go to the stable kernels?
>> > > > Yes, this needs to go to stable kernels also as this fixes a
>> > > > potential issue
>> > > > which is easily reproducible.
>> > >
>> > >
>> > >
>> > > >
>> > > > >
>> > > > > > ---
>> > > > > > Changes in v3:
>> > > > > > - relocate patch change list
>> > > > > >
>> > > > > > Changes in v2:
>> > > > > > - updated commit message to proper format
>> > > > > > - added fixes tag to commit message
>> > > > > > - removed unnecessary variable initialization
>> > > > > > - removed length check during payload calculation
>> > > > > >
>> > > > > >  drivers/misc/fastrpc.c | 10 ++++++----
>> > > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
>> > > > > >
>> > > > > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> > > > > > index beda610..69d45c4 100644
>> > > > > > --- a/drivers/misc/fastrpc.c
>> > > > > > +++ b/drivers/misc/fastrpc.c
>> > > > > > @@ -719,16 +719,18 @@ 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;
>> > > > >
>> > > > > What does "oix" stand for?  What was wrong with i?
>> > > > It is just a general convention we use. "oix" is used to iterate
>> > > > through
>> > > > sorted overlap buffer list and use "i" to get corresponding
>> > > > unsorted list
>> > > > index. We follow the same convention at other places also, for
>> > > > example:
>> > > > fastrpc_get_args function.
>> > >
>> > > That is the only place it is used in all of the whole kernel tree.  It
>> > > is not a normal variable for a loop, so who is "we" here?
>> > The convention was followed for the same file(fastrpc.c). As part of
>> > fastrpc_get_args
>> > function, while iterating through sorted buffer list, oix is used as
>> > index and to
>> > get unsorted index "raix", it is using "i". Just following the same way
>> > here to
>> > have better understanding. Please let me know if this is a concern, it
>> > can be updated
>> > to "i", "j" etc.
>> >
>> > -- Thanks
>> > >
>> > > thanks,
>> > >
>> > > greg k-h
>> Hello Greg,
>> 
>> Is this bug-fix patch planned to be released?
> 
> Released in what way?
By release, I mean picked to your misc driver git tree.
> 
> I do not see it in any tree anywhere, perhaps it needs to be 
> resubmitted
> to be accepted?
Sure, will resubmit the patch. Thanks.
> 
> thanks,
> 
> greg k-h