Received-SPF: pass (google.com: domain of linux-kernel+bounces-198630-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <1272b70e-fe2f-47cd-af77-6d3461b8596c@quicinc.com>
Date: Mon, 3 Jun 2024 11:45:26 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/9] misc: fastrpc: Fix DSP capabilities request
Content-Language: en-US
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: <srinivas.kandagatla@linaro.org>, <linux-arm-msm@vger.kernel.org>,
        <gregkh@linuxfoundation.org>, <quic_bkumar@quicinc.com>,
        <linux-kernel@vger.kernel.org>, <quic_chennak@quicinc.com>,
        stable
	<stable@kernel.org>
References: <20240530102032.27179-1-quic_ekangupt@quicinc.com>
 <20240530102032.27179-3-quic_ekangupt@quicinc.com>
 <zcqc4dgc6pippwiysybmkbvogfd6gbinnrw65kiulie3wlup5y@wq4dexvamo7t>
From: Ekansh Gupta <quic_ekangupt@quicinc.com>
In-Reply-To: <zcqc4dgc6pippwiysybmkbvogfd6gbinnrw65kiulie3wlup5y@wq4dexvamo7t>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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@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);
> 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.

>
>>   	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
>>