| Message ID | 1632125731-18768-1-git-send-email-jeyr@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | misc: fastrpc: fix improper packet size calculation | expand |
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
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".
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
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); > } > } > >
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); } }
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(-)