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@xxxxxxxxxx>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
> >> ---
> >> 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
