Re: [PATCH v3 2/9] misc: fastrpc: Fix DSP capabilities request

[Dmitry Baryshkov]

On Mon, Jun 03, 2024 at 11:45:26AM +0530, Ekansh Gupta wrote:
> 
> On 5/30/2024 4:29 PM, Dmitry Baryshkov wrote:
> > On Thu, May 30, 2024 at 03:50:20PM +0530, Ekansh Gupta wrote:
> > > Incorrect remote arguments are getting passed when requesting for
> > > capabilities from DSP.
> > Describe why and how they are incorrect.
> 
> Sure, I'll update this information in the next spin.
> 
> > 
> > > Also there is no requirement to update the
> > > PD type as it might cause problems for any PD other than user PD.
> > Also... means that these are two separate issues. There should be two
> > separate commits.
> 
> Okay, I'll separate out the PD type change.
> 
> > 
> > > In addition to this, the collected capability information is not
> > > getting copied properly to user. Add changes to address these
> > > problems and get correct DSP capabilities.
> > > 
> > > Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities")
> > > Cc: stable <stable@xxxxxxxxxx>
> > > Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
> > > ---
> > >   drivers/misc/fastrpc.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > index 4028cb96bcf2..61389795f498 100644
> > > --- a/drivers/misc/fastrpc.c
> > > +++ b/drivers/misc/fastrpc.c
> > > @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr
> > >   	args[0].length = sizeof(dsp_attr_buf_len);
> > >   	args[0].fd = -1;
> > >   	args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1];
> > > -	args[1].length = dsp_attr_buf_len;
> > > +	args[1].length = dsp_attr_buf_len * sizeof(uint32_t);
> > As you are skipping first entry, should there be (dsp_attr_buf_len - 1)
> > * sizeof(uint32_t).
> 
> This was done in the next patch of the series, I'll bring it here.
> 
> > 
> > >   	args[1].fd = -1;
> > > -	fl->pd = USER_PD;
> > >   	return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE,
> > >   				       FASTRPC_SCALARS(0, 1, 1), args);
> > > @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> > >   	if (!dsp_attributes)
> > >   		return -ENOMEM;
> > > -	err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN);
> > > +	err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
> > So it looks like the argument was correct. It was passing length, not
> > the number of attributes. The only thing to fix is that args[1].length
> > should be dsp_attr_buf_len - sizeof(*dsp_attr_buf).
> 
> args[0] is expected to carry the information about the total number of attributes to be copied from DSP
> and not the information about the size to be copied. Passing the size information leads to a failure
> suggesting bad arguments passed to DSP.

AH, so it gets passed twice. As a pointer to u64 (for the number ofh
attributes) and as a size for those attributes (via args[1].length).

Please explain this in the commit message.

> 
> > 
> > >   	if (err == DSP_UNSUPPORTED_API) {
> > >   		dev_info(&cctx->rpdev->dev,
> > >   			 "Warning: DSP capabilities not supported on domain: %d\n", domain);
> > > @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> > >   	if (err)
> > >   		return err;
> > > -	if (copy_to_user(argp, &cap.capability, sizeof(cap.capability)))
> > > +	if (copy_to_user(argp, &cap, sizeof(cap)))
> > >   		return -EFAULT;
> > >   	return 0;
> > > -- 
> > > 2.43.0
> > > 

-- 
With best wishes
Dmitry