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

[Ekansh Gupta <quic_ekangupt@quicinc.com>]

On 5/31/2024 3:03 PM, Srinivas Kandagatla wrote:
>
>
> On 30/05/2024 11:20, Ekansh Gupta wrote:
>> Incorrect remote arguments are getting passed when requesting for
>> capabilities from DSP. Also there is no requirement to update the
>> PD type as it might cause problems for any PD other than user PD.
>> 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@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   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);
> This does not look correct,
>
> we have allocated buffer of size FASTRPC_MAX_DSP_ATTRIBUTES_LEN which is
> already (sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES)
>
> now this patch multiplies with again sizeof(uint32_t), this is going to send dsp incorrect size for buffer and overrun the buffer size.
As the argument passed to this function is number of attributes instead of length, this won't cause another multiplication with (uint32_t).
>
>
>
>>       args[1].fd = -1;
>> -    fl->pd = USER_PD;
> another patch may be.
Sure.
>
>>         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);
>
> You change this again to send FASTRPC_MAX_DSP_ATTRIBUTES instead of FASTRPC_MAX_DSP_ATTRIBUTES_LEN but why?
Copying the comment sent to Dmitry's queries:
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.
>
>
>>       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)))
>
> Why are we copying the full struct here? All that user needs is cap.capability?
as argp sent from user during ioctl is the capability structure, the same argp is copied to a local fastrpc_ioctl_capability structure(cap) to get the domain and attribute_id information. Copying just the capability member to argp will cause problem when the user tries to read the capability. While testing the capability, I was observing this failure and it is resolved once we copy the information properly.
>
>
>
> --srini
>
>
>>           return -EFAULT;
>>         return 0;
>