[PATCH v2] misc: fastrpc: Remove user PD initmem size check

[PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Ekansh Gupta]

For user PD initialization, initmem is allocated and sent to DSP for
initial memory requirements like shell loading. This size is passed
by user space and is checked against a max size. For unsigned PD
offloading, more than 2MB size could be passed by user which would
result in PD initialization failure. Remove the user PD initmem size
check and allow buffer allocation for user passed size. Any additional
memory sent to DSP during PD init is used as the PD heap.

Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
Changes in v2:
  - Modified commit text.
  - Removed size check instead of updating max file size.

 drivers/misc/fastrpc.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 5204fda51da3..9d064deeac89 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1389,11 +1389,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 		goto err;
 	}
 
-	if (init.filelen > INIT_FILELEN_MAX) {
-		err = -EINVAL;
-		goto err;
-	}
-
 	inbuf.pgid = fl->tgid;
 	inbuf.namelen = strlen(current->comm) + 1;
 	inbuf.filelen = init.filelen;
-- 
2.34.1

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Dmitry Baryshkov]

On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
> For user PD initialization, initmem is allocated and sent to DSP for
> initial memory requirements like shell loading. This size is passed
> by user space and is checked against a max size. For unsigned PD
> offloading, more than 2MB size could be passed by user which would
> result in PD initialization failure. Remove the user PD initmem size
> check and allow buffer allocation for user passed size. Any additional
> memory sent to DSP during PD init is used as the PD heap.

Would it allow malicious userspace to allocate big enough buffers and
reduce the amount of memory available to the system? To other DSP
programs?

> 
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
> Changes in v2:
>   - Modified commit text.
>   - Removed size check instead of updating max file size.
> 
>  drivers/misc/fastrpc.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 5204fda51da3..9d064deeac89 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1389,11 +1389,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  		goto err;
>  	}
>  
> -	if (init.filelen > INIT_FILELEN_MAX) {
> -		err = -EINVAL;
> -		goto err;
> -	}
> -
>  	inbuf.pgid = fl->tgid;
>  	inbuf.namelen = strlen(current->comm) + 1;
>  	inbuf.filelen = init.filelen;
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Ekansh Gupta]

On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
>> For user PD initialization, initmem is allocated and sent to DSP for
>> initial memory requirements like shell loading. This size is passed
>> by user space and is checked against a max size. For unsigned PD
>> offloading, more than 2MB size could be passed by user which would
>> result in PD initialization failure. Remove the user PD initmem size
>> check and allow buffer allocation for user passed size. Any additional
>> memory sent to DSP during PD init is used as the PD heap.
> Would it allow malicious userspace to allocate big enough buffers and
> reduce the amount of memory available to the system? To other DSP
> programs?
The allocation here is happening from SMMU context bank which is uniquely assigned
to processes going to DSP. As per my understanding process can allocate maximum
4GB of memory from the context bank and the memory availability will be taken care
by kernel memory management. Please correct me if my understanding is incorrect.

--Ekansh
>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>> Changes in v2:
>>   - Modified commit text.
>>   - Removed size check instead of updating max file size.
>>
>>  drivers/misc/fastrpc.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 5204fda51da3..9d064deeac89 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1389,11 +1389,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>  		goto err;
>>  	}
>>  
>> -	if (init.filelen > INIT_FILELEN_MAX) {
>> -		err = -EINVAL;
>> -		goto err;
>> -	}
>> -
>>  	inbuf.pgid = fl->tgid;
>>  	inbuf.namelen = strlen(current->comm) + 1;
>>  	inbuf.filelen = init.filelen;
>> -- 
>> 2.34.1
>>

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Ekansh Gupta]

On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
>
> On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
>> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
>>> For user PD initialization, initmem is allocated and sent to DSP for
>>> initial memory requirements like shell loading. This size is passed
>>> by user space and is checked against a max size. For unsigned PD
>>> offloading, more than 2MB size could be passed by user which would
>>> result in PD initialization failure. Remove the user PD initmem size
>>> check and allow buffer allocation for user passed size. Any additional
>>> memory sent to DSP during PD init is used as the PD heap.
>> Would it allow malicious userspace to allocate big enough buffers and
>> reduce the amount of memory available to the system? To other DSP
>> programs?
> The allocation here is happening from SMMU context bank which is uniquely assigned
> to processes going to DSP. As per my understanding process can allocate maximum
> 4GB of memory from the context bank and the memory availability will be taken care
> by kernel memory management. Please correct me if my understanding is incorrect.
Just wanted to add 1 question here:
User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
allocated huge memory? Is there any bound check or is it handled differently? Please help me with
some information here.
> --Ekansh
>>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>>> Cc: stable <stable@kernel.org>
>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>> ---
>>> Changes in v2:
>>>   - Modified commit text.
>>>   - Removed size check instead of updating max file size.
>>>
>>>  drivers/misc/fastrpc.c | 5 -----
>>>  1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 5204fda51da3..9d064deeac89 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -1389,11 +1389,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>>  		goto err;
>>>  	}
>>>  
>>> -	if (init.filelen > INIT_FILELEN_MAX) {
>>> -		err = -EINVAL;
>>> -		goto err;
>>> -	}
>>> -
>>>  	inbuf.pgid = fl->tgid;
>>>  	inbuf.namelen = strlen(current->comm) + 1;
>>>  	inbuf.filelen = init.filelen;
>>> -- 
>>> 2.34.1
>>>
>

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Greg KH]

On Fri, Jun 28, 2024 at 04:12:10PM +0530, Ekansh Gupta wrote:
> 
> 
> On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
> >
> > On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
> >> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
> >>> For user PD initialization, initmem is allocated and sent to DSP for
> >>> initial memory requirements like shell loading. This size is passed
> >>> by user space and is checked against a max size. For unsigned PD
> >>> offloading, more than 2MB size could be passed by user which would
> >>> result in PD initialization failure. Remove the user PD initmem size
> >>> check and allow buffer allocation for user passed size. Any additional
> >>> memory sent to DSP during PD init is used as the PD heap.
> >> Would it allow malicious userspace to allocate big enough buffers and
> >> reduce the amount of memory available to the system? To other DSP
> >> programs?
> > The allocation here is happening from SMMU context bank which is uniquely assigned
> > to processes going to DSP. As per my understanding process can allocate maximum
> > 4GB of memory from the context bank and the memory availability will be taken care
> > by kernel memory management. Please correct me if my understanding is incorrect.
> Just wanted to add 1 question here:
> User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
> allocated huge memory?

No, because any userspace program that takes up too much memory will be
killed by the kernel.

You can not have userspace tell the kernel to allocate 100Gb of memory,
as then the kernel is the one that just took it all up, and then
userspace applications will start to be killed off.

You MUST bounds check your userspace-supplied memory requests.  Remember
the 4 words of kernel development:

	All input is evil.

thanks,

greg k-h

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Ekansh Gupta]

On 6/28/2024 7:51 PM, Greg KH wrote:
> On Fri, Jun 28, 2024 at 04:12:10PM +0530, Ekansh Gupta wrote:
>>
>> On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
>>> On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
>>>> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
>>>>> For user PD initialization, initmem is allocated and sent to DSP for
>>>>> initial memory requirements like shell loading. This size is passed
>>>>> by user space and is checked against a max size. For unsigned PD
>>>>> offloading, more than 2MB size could be passed by user which would
>>>>> result in PD initialization failure. Remove the user PD initmem size
>>>>> check and allow buffer allocation for user passed size. Any additional
>>>>> memory sent to DSP during PD init is used as the PD heap.
>>>> Would it allow malicious userspace to allocate big enough buffers and
>>>> reduce the amount of memory available to the system? To other DSP
>>>> programs?
>>> The allocation here is happening from SMMU context bank which is uniquely assigned
>>> to processes going to DSP. As per my understanding process can allocate maximum
>>> 4GB of memory from the context bank and the memory availability will be taken care
>>> by kernel memory management. Please correct me if my understanding is incorrect.
>> Just wanted to add 1 question here:
>> User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
>> allocated huge memory?
> No, because any userspace program that takes up too much memory will be
> killed by the kernel.
>
> You can not have userspace tell the kernel to allocate 100Gb of memory,
> as then the kernel is the one that just took it all up, and then
> userspace applications will start to be killed off.
>
> You MUST bounds check your userspace-supplied memory requests.  Remember
> the 4 words of kernel development:
>
> 	All input is evil.
Thanks for the detailed explanation, Greg. I'll remember this going forward.

For this change, I'll increase the max size limit to 5MB which is the requirement for
unsigned PD to run on DSP.

--Ekansh
> thanks,
>
> greg k-h

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Dmitry Baryshkov]

On Mon, Jul 01, 2024 at 10:50:38AM GMT, Ekansh Gupta wrote:
> 
> 
> On 6/28/2024 7:51 PM, Greg KH wrote:
> > On Fri, Jun 28, 2024 at 04:12:10PM +0530, Ekansh Gupta wrote:
> >>
> >> On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
> >>> On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
> >>>> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
> >>>>> For user PD initialization, initmem is allocated and sent to DSP for
> >>>>> initial memory requirements like shell loading. This size is passed
> >>>>> by user space and is checked against a max size. For unsigned PD
> >>>>> offloading, more than 2MB size could be passed by user which would
> >>>>> result in PD initialization failure. Remove the user PD initmem size
> >>>>> check and allow buffer allocation for user passed size. Any additional
> >>>>> memory sent to DSP during PD init is used as the PD heap.
> >>>> Would it allow malicious userspace to allocate big enough buffers and
> >>>> reduce the amount of memory available to the system? To other DSP
> >>>> programs?
> >>> The allocation here is happening from SMMU context bank which is uniquely assigned
> >>> to processes going to DSP. As per my understanding process can allocate maximum
> >>> 4GB of memory from the context bank and the memory availability will be taken care
> >>> by kernel memory management. Please correct me if my understanding is incorrect.
> >> Just wanted to add 1 question here:
> >> User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
> >> allocated huge memory?
> > No, because any userspace program that takes up too much memory will be
> > killed by the kernel.
> >
> > You can not have userspace tell the kernel to allocate 100Gb of memory,
> > as then the kernel is the one that just took it all up, and then
> > userspace applications will start to be killed off.
> >
> > You MUST bounds check your userspace-supplied memory requests.  Remember
> > the 4 words of kernel development:
> >
> > 	All input is evil.
> Thanks for the detailed explanation, Greg. I'll remember this going forward.
> 
> For this change, I'll increase the max size limit to 5MB which is the requirement for
> unsigned PD to run on DSP.

So we are back to the quesiton of why 5MB is considered to be enough,
see

https://lore.kernel.org/linux-arm-msm/2024061755-snare-french-de38@gregkh/

> 
> --Ekansh
> > thanks,
> >
> > greg k-h
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Ekansh Gupta]

On 7/1/2024 10:41 PM, Dmitry Baryshkov wrote:
> On Mon, Jul 01, 2024 at 10:50:38AM GMT, Ekansh Gupta wrote:
>>
>> On 6/28/2024 7:51 PM, Greg KH wrote:
>>> On Fri, Jun 28, 2024 at 04:12:10PM +0530, Ekansh Gupta wrote:
>>>> On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
>>>>> On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
>>>>>> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
>>>>>>> For user PD initialization, initmem is allocated and sent to DSP for
>>>>>>> initial memory requirements like shell loading. This size is passed
>>>>>>> by user space and is checked against a max size. For unsigned PD
>>>>>>> offloading, more than 2MB size could be passed by user which would
>>>>>>> result in PD initialization failure. Remove the user PD initmem size
>>>>>>> check and allow buffer allocation for user passed size. Any additional
>>>>>>> memory sent to DSP during PD init is used as the PD heap.
>>>>>> Would it allow malicious userspace to allocate big enough buffers and
>>>>>> reduce the amount of memory available to the system? To other DSP
>>>>>> programs?
>>>>> The allocation here is happening from SMMU context bank which is uniquely assigned
>>>>> to processes going to DSP. As per my understanding process can allocate maximum
>>>>> 4GB of memory from the context bank and the memory availability will be taken care
>>>>> by kernel memory management. Please correct me if my understanding is incorrect.
>>>> Just wanted to add 1 question here:
>>>> User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
>>>> allocated huge memory?
>>> No, because any userspace program that takes up too much memory will be
>>> killed by the kernel.
>>>
>>> You can not have userspace tell the kernel to allocate 100Gb of memory,
>>> as then the kernel is the one that just took it all up, and then
>>> userspace applications will start to be killed off.
>>>
>>> You MUST bounds check your userspace-supplied memory requests.  Remember
>>> the 4 words of kernel development:
>>>
>>> 	All input is evil.
>> Thanks for the detailed explanation, Greg. I'll remember this going forward.
>>
>> For this change, I'll increase the max size limit to 5MB which is the requirement for
>> unsigned PD to run on DSP.
> So we are back to the quesiton of why 5MB is considered to be enough,
> see
>
> https://lore.kernel.org/linux-arm-msm/2024061755-snare-french-de38@gregkh/
This is based on the initial memory requirement for unsigned PD. This includes memory for shell loading on DSP
+ memory for static heap allocations(heap allocations are dynamic for Signed PD). This requirement tends to
around 5MB. I'll update this  also information in commit text. There will be some additional memory passed to
the PD which will get added to the PD heap.

--Ekansh
>
>> --Ekansh
>>> thanks,
>>>
>>> greg k-h

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Dmitry Baryshkov]

On Tue, 2 Jul 2024 at 10:07, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>
>
>
> On 7/1/2024 10:41 PM, Dmitry Baryshkov wrote:
> > On Mon, Jul 01, 2024 at 10:50:38AM GMT, Ekansh Gupta wrote:
> >>
> >> On 6/28/2024 7:51 PM, Greg KH wrote:
> >>> On Fri, Jun 28, 2024 at 04:12:10PM +0530, Ekansh Gupta wrote:
> >>>> On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
> >>>>> On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
> >>>>>> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
> >>>>>>> For user PD initialization, initmem is allocated and sent to DSP for
> >>>>>>> initial memory requirements like shell loading. This size is passed
> >>>>>>> by user space and is checked against a max size. For unsigned PD
> >>>>>>> offloading, more than 2MB size could be passed by user which would
> >>>>>>> result in PD initialization failure. Remove the user PD initmem size
> >>>>>>> check and allow buffer allocation for user passed size. Any additional
> >>>>>>> memory sent to DSP during PD init is used as the PD heap.
> >>>>>> Would it allow malicious userspace to allocate big enough buffers and
> >>>>>> reduce the amount of memory available to the system? To other DSP
> >>>>>> programs?
> >>>>> The allocation here is happening from SMMU context bank which is uniquely assigned
> >>>>> to processes going to DSP. As per my understanding process can allocate maximum
> >>>>> 4GB of memory from the context bank and the memory availability will be taken care
> >>>>> by kernel memory management. Please correct me if my understanding is incorrect.
> >>>> Just wanted to add 1 question here:
> >>>> User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
> >>>> allocated huge memory?
> >>> No, because any userspace program that takes up too much memory will be
> >>> killed by the kernel.
> >>>
> >>> You can not have userspace tell the kernel to allocate 100Gb of memory,
> >>> as then the kernel is the one that just took it all up, and then
> >>> userspace applications will start to be killed off.
> >>>
> >>> You MUST bounds check your userspace-supplied memory requests.  Remember
> >>> the 4 words of kernel development:
> >>>
> >>>     All input is evil.
> >> Thanks for the detailed explanation, Greg. I'll remember this going forward.
> >>
> >> For this change, I'll increase the max size limit to 5MB which is the requirement for
> >> unsigned PD to run on DSP.
> > So we are back to the quesiton of why 5MB is considered to be enough,
> > see
> >
> > https://lore.kernel.org/linux-arm-msm/2024061755-snare-french-de38@gregkh/
> This is based on the initial memory requirement for unsigned PD. This includes memory for shell loading on DSP
> + memory for static heap allocations(heap allocations are dynamic for Signed PD). This requirement tends to
> around 5MB. I'll update this  also information in commit text. There will be some additional memory passed to
> the PD which will get added to the PD heap.

Could you please clarify, are these 2MB and 5MB requirements coming
from the DSP side or from the userspace side? In other words, is it
coming from the shell / firmware / etc?

>
> --Ekansh
> >
> >> --Ekansh
> >>> thanks,
> >>>
> >>> greg k-h
>


-- 
With best wishes
Dmitry

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Ekansh Gupta]

On 7/2/2024 3:10 PM, Dmitry Baryshkov wrote:
> On Tue, 2 Jul 2024 at 10:07, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>>
>>
>> On 7/1/2024 10:41 PM, Dmitry Baryshkov wrote:
>>> On Mon, Jul 01, 2024 at 10:50:38AM GMT, Ekansh Gupta wrote:
>>>> On 6/28/2024 7:51 PM, Greg KH wrote:
>>>>> On Fri, Jun 28, 2024 at 04:12:10PM +0530, Ekansh Gupta wrote:
>>>>>> On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
>>>>>>> On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
>>>>>>>>> For user PD initialization, initmem is allocated and sent to DSP for
>>>>>>>>> initial memory requirements like shell loading. This size is passed
>>>>>>>>> by user space and is checked against a max size. For unsigned PD
>>>>>>>>> offloading, more than 2MB size could be passed by user which would
>>>>>>>>> result in PD initialization failure. Remove the user PD initmem size
>>>>>>>>> check and allow buffer allocation for user passed size. Any additional
>>>>>>>>> memory sent to DSP during PD init is used as the PD heap.
>>>>>>>> Would it allow malicious userspace to allocate big enough buffers and
>>>>>>>> reduce the amount of memory available to the system? To other DSP
>>>>>>>> programs?
>>>>>>> The allocation here is happening from SMMU context bank which is uniquely assigned
>>>>>>> to processes going to DSP. As per my understanding process can allocate maximum
>>>>>>> 4GB of memory from the context bank and the memory availability will be taken care
>>>>>>> by kernel memory management. Please correct me if my understanding is incorrect.
>>>>>> Just wanted to add 1 question here:
>>>>>> User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
>>>>>> allocated huge memory?
>>>>> No, because any userspace program that takes up too much memory will be
>>>>> killed by the kernel.
>>>>>
>>>>> You can not have userspace tell the kernel to allocate 100Gb of memory,
>>>>> as then the kernel is the one that just took it all up, and then
>>>>> userspace applications will start to be killed off.
>>>>>
>>>>> You MUST bounds check your userspace-supplied memory requests.  Remember
>>>>> the 4 words of kernel development:
>>>>>
>>>>>     All input is evil.
>>>> Thanks for the detailed explanation, Greg. I'll remember this going forward.
>>>>
>>>> For this change, I'll increase the max size limit to 5MB which is the requirement for
>>>> unsigned PD to run on DSP.
>>> So we are back to the quesiton of why 5MB is considered to be enough,
>>> see
>>>
>>> https://lore.kernel.org/linux-arm-msm/2024061755-snare-french-de38@gregkh/
>> This is based on the initial memory requirement for unsigned PD. This includes memory for shell loading on DSP
>> + memory for static heap allocations(heap allocations are dynamic for Signed PD). This requirement tends to
>> around 5MB. I'll update this  also information in commit text. There will be some additional memory passed to
>> the PD which will get added to the PD heap.
> Could you please clarify, are these 2MB and 5MB requirements coming
> from the DSP side or from the userspace side? In other words, is it
> coming from the shell / firmware / etc?
I did some more checking here, I'll summarize the problem and try to propose a
better solution:
init.filelen is actually the size of fastrpc shell file which is close to 900kb for both
signed and unsigned shells. User space passes this memory and size after opening
and reading the shell file. The bound check is for this filelen which looks correct also
as this size is not expected to be more than 2MB.

Now for PD initmem, this memory is needed for PD initialization which includes
loading of shell and other initialization requirements eg. PD heap. As of today, the
initmem allocation is taken as the max of FASTRPC_FILELEN_MAX(2MB bound check
macro) and 4 times of filelen(~4MB). So every time, atleast 2MB memory is allocated
for this initmem for PD initialization.

For unsigned PD, there are some additional read-write segments loaded on DSP which
takes the size requirement to slightly more than 4MB. Therefore allocating 5MB is
helping unsigned PD to be spawned.

I hope this helps in understanding the problem.

Proposed solution:
I believe the bound check macro should not be used for initmem size. I can add some
new definition with FASTRPC_INITMEM_MIN.

In the create_process function, the "memlen" will be set to FASTRPC_INITMEM_MIN
initially and it will get increased to 5MB if "unsigned_module" is requested.

Or I can add different MACRO definitions for both signed(3MB) and unsigned PD(5MB)
minimum initmem size.

Please let me know if this is not clear, I'll send a patch for better understanding.

--Ekansh

>> --Ekansh
>>>> --Ekansh
>>>>> thanks,
>>>>>
>>>>> greg k-h
>

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Dmitry Baryshkov]

On Wed, 3 Jul 2024 at 09:44, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>
>
>
> On 7/2/2024 3:10 PM, Dmitry Baryshkov wrote:
> > On Tue, 2 Jul 2024 at 10:07, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
> >>
> >>
> >> On 7/1/2024 10:41 PM, Dmitry Baryshkov wrote:
> >>> On Mon, Jul 01, 2024 at 10:50:38AM GMT, Ekansh Gupta wrote:
> >>>> On 6/28/2024 7:51 PM, Greg KH wrote:
> >>>>> On Fri, Jun 28, 2024 at 04:12:10PM +0530, Ekansh Gupta wrote:
> >>>>>> On 6/28/2024 3:59 PM, Ekansh Gupta wrote:
> >>>>>>> On 6/27/2024 4:43 PM, Dmitry Baryshkov wrote:
> >>>>>>>> On Thu, Jun 27, 2024 at 11:35:18AM GMT, Ekansh Gupta wrote:
> >>>>>>>>> For user PD initialization, initmem is allocated and sent to DSP for
> >>>>>>>>> initial memory requirements like shell loading. This size is passed
> >>>>>>>>> by user space and is checked against a max size. For unsigned PD
> >>>>>>>>> offloading, more than 2MB size could be passed by user which would
> >>>>>>>>> result in PD initialization failure. Remove the user PD initmem size
> >>>>>>>>> check and allow buffer allocation for user passed size. Any additional
> >>>>>>>>> memory sent to DSP during PD init is used as the PD heap.
> >>>>>>>> Would it allow malicious userspace to allocate big enough buffers and
> >>>>>>>> reduce the amount of memory available to the system? To other DSP
> >>>>>>>> programs?
> >>>>>>> The allocation here is happening from SMMU context bank which is uniquely assigned
> >>>>>>> to processes going to DSP. As per my understanding process can allocate maximum
> >>>>>>> 4GB of memory from the context bank and the memory availability will be taken care
> >>>>>>> by kernel memory management. Please correct me if my understanding is incorrect.
> >>>>>> Just wanted to add 1 question here:
> >>>>>> User space can also directly allocate memory. Wouldn't that be a problem if any malicious userspace
> >>>>>> allocated huge memory?
> >>>>> No, because any userspace program that takes up too much memory will be
> >>>>> killed by the kernel.
> >>>>>
> >>>>> You can not have userspace tell the kernel to allocate 100Gb of memory,
> >>>>> as then the kernel is the one that just took it all up, and then
> >>>>> userspace applications will start to be killed off.
> >>>>>
> >>>>> You MUST bounds check your userspace-supplied memory requests.  Remember
> >>>>> the 4 words of kernel development:
> >>>>>
> >>>>>     All input is evil.
> >>>> Thanks for the detailed explanation, Greg. I'll remember this going forward.
> >>>>
> >>>> For this change, I'll increase the max size limit to 5MB which is the requirement for
> >>>> unsigned PD to run on DSP.
> >>> So we are back to the quesiton of why 5MB is considered to be enough,
> >>> see
> >>>
> >>> https://lore.kernel.org/linux-arm-msm/2024061755-snare-french-de38@gregkh/
> >> This is based on the initial memory requirement for unsigned PD. This includes memory for shell loading on DSP
> >> + memory for static heap allocations(heap allocations are dynamic for Signed PD). This requirement tends to
> >> around 5MB. I'll update this  also information in commit text. There will be some additional memory passed to
> >> the PD which will get added to the PD heap.
> > Could you please clarify, are these 2MB and 5MB requirements coming
> > from the DSP side or from the userspace side? In other words, is it
> > coming from the shell / firmware / etc?
> I did some more checking here, I'll summarize the problem and try to propose a
> better solution:
> init.filelen is actually the size of fastrpc shell file which is close to 900kb for both
> signed and unsigned shells. User space passes this memory and size after opening
> and reading the shell file. The bound check is for this filelen which looks correct also
> as this size is not expected to be more than 2MB.
>
> Now for PD initmem, this memory is needed for PD initialization which includes
> loading of shell and other initialization requirements eg. PD heap. As of today, the
> initmem allocation is taken as the max of FASTRPC_FILELEN_MAX(2MB bound check
> macro) and 4 times of filelen(~4MB). So every time, atleast 2MB memory is allocated
> for this initmem for PD initialization.

Why are you using 4x here? Shouldn't the heap size be a const or at
least more or less independent from the object size?
Also the object size probably doesn't include the BSS size of the
file. So most likely the allocated memory size should be sum(p_memsz
for each PH) + heap size.

>
> For unsigned PD, there are some additional read-write segments loaded on DSP which
> takes the size requirement to slightly more than 4MB. Therefore allocating 5MB is
> helping unsigned PD to be spawned.

Which read-write segments?

> I hope this helps in understanding the problem.
>
> Proposed solution:
> I believe the bound check macro should not be used for initmem size. I can add some
> new definition with FASTRPC_INITMEM_MIN.
>
> In the create_process function, the "memlen" will be set to FASTRPC_INITMEM_MIN
> initially and it will get increased to 5MB if "unsigned_module" is requested.
>
> Or I can add different MACRO definitions for both signed(3MB) and unsigned PD(5MB)
> minimum initmem size.

From your description I'd prefer to have:

#define FASTRPC_EXTRA_FOR_READWRITE_WHATEVER 3MB
if (unsigned_pd)
   initmem_size += FASTRPC_EXTRA....;


>
> Please let me know if this is not clear, I'll send a patch for better understanding.
>
> --Ekansh
>
> >> --Ekansh
> >>>> --Ekansh
> >>>>> thanks,
> >>>>>
> >>>>> greg k-h
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Greg KH]

On Thu, Jun 27, 2024 at 11:35:18AM +0530, Ekansh Gupta wrote:
> For user PD initialization, initmem is allocated and sent to DSP for
> initial memory requirements like shell loading. This size is passed
> by user space and is checked against a max size. For unsigned PD
> offloading, more than 2MB size could be passed by user which would
> result in PD initialization failure. Remove the user PD initmem size
> check and allow buffer allocation for user passed size. Any additional
> memory sent to DSP during PD init is used as the PD heap.
> 
> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
> Changes in v2:
>   - Modified commit text.
>   - Removed size check instead of updating max file size.
> 
>  drivers/misc/fastrpc.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 5204fda51da3..9d064deeac89 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1389,11 +1389,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>  		goto err;
>  	}
>  
> -	if (init.filelen > INIT_FILELEN_MAX) {
> -		err = -EINVAL;
> -		goto err;
> -	}
> -
>  	inbuf.pgid = fl->tgid;
>  	inbuf.namelen = strlen(current->comm) + 1;
>  	inbuf.filelen = init.filelen;

This feels really wrong as now there is no way to bounds-check the
buffer size at all, so userspace can do "bad things" like go over the
defined buffer size limit which you are expecting, right?

So how is this actually correct?  If you want larger sizes, then
increase the INIT_FILELEN_MAX value.

thanks,

greg k-h

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Greg KH]

On Thu, Jun 27, 2024 at 01:20:15PM +0200, Greg KH wrote:
> On Thu, Jun 27, 2024 at 11:35:18AM +0530, Ekansh Gupta wrote:
> > For user PD initialization, initmem is allocated and sent to DSP for
> > initial memory requirements like shell loading. This size is passed
> > by user space and is checked against a max size. For unsigned PD
> > offloading, more than 2MB size could be passed by user which would
> > result in PD initialization failure. Remove the user PD initmem size
> > check and allow buffer allocation for user passed size. Any additional
> > memory sent to DSP during PD init is used as the PD heap.
> > 
> > Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > ---
> > Changes in v2:
> >   - Modified commit text.
> >   - Removed size check instead of updating max file size.
> > 
> >  drivers/misc/fastrpc.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > index 5204fda51da3..9d064deeac89 100644
> > --- a/drivers/misc/fastrpc.c
> > +++ b/drivers/misc/fastrpc.c
> > @@ -1389,11 +1389,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> >  		goto err;
> >  	}
> >  
> > -	if (init.filelen > INIT_FILELEN_MAX) {
> > -		err = -EINVAL;
> > -		goto err;
> > -	}
> > -
> >  	inbuf.pgid = fl->tgid;
> >  	inbuf.namelen = strlen(current->comm) + 1;
> >  	inbuf.filelen = init.filelen;
> 
> This feels really wrong as now there is no way to bounds-check the
> buffer size at all, so userspace can do "bad things" like go over the
> defined buffer size limit which you are expecting, right?
> 
> So how is this actually correct?  If you want larger sizes, then
> increase the INIT_FILELEN_MAX value.

And this feels really wrong compared to v1 of this patch, which did
attempt to increase the size, yet it wasn't really well defined.  Now
you went way too far to allow ANY size to be passed in here, which is
obviously what you don't want to do.

thanks,

greg k-h

Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check

[Ekansh Gupta]

On 6/27/2024 4:50 PM, Greg KH wrote:
> On Thu, Jun 27, 2024 at 11:35:18AM +0530, Ekansh Gupta wrote:
>> For user PD initialization, initmem is allocated and sent to DSP for
>> initial memory requirements like shell loading. This size is passed
>> by user space and is checked against a max size. For unsigned PD
>> offloading, more than 2MB size could be passed by user which would
>> result in PD initialization failure. Remove the user PD initmem size
>> check and allow buffer allocation for user passed size. Any additional
>> memory sent to DSP during PD init is used as the PD heap.
>>
>> Fixes: 7f1f481263c3 ("misc: fastrpc: check before loading process to the DSP")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>> Changes in v2:
>>   - Modified commit text.
>>   - Removed size check instead of updating max file size.
>>
>>  drivers/misc/fastrpc.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 5204fda51da3..9d064deeac89 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1389,11 +1389,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>>  		goto err;
>>  	}
>>  
>> -	if (init.filelen > INIT_FILELEN_MAX) {
>> -		err = -EINVAL;
>> -		goto err;
>> -	}
>> -
>>  	inbuf.pgid = fl->tgid;
>>  	inbuf.namelen = strlen(current->comm) + 1;
>>  	inbuf.filelen = init.filelen;
> This feels really wrong as now there is no way to bounds-check the
> buffer size at all, so userspace can do "bad things" like go over the
> defined buffer size limit which you are expecting, right?
>
> So how is this actually correct?  If you want larger sizes, then
> increase the INIT_FILELEN_MAX value.
The reason for removing this check is that I don't know how much memory can
any user process ask for DSP PD initialization. There are some cases where huge
memory requirement can come. As for the expectation, any memory allocated here
will get completely added to DSP PD. I understand your concern but can you please
suggest any way I can have some bound-check to handle this also(requirement more
than 5MB)? The memory is allocated using SMMU context banks which is uniquely
assigned to process so the max allocation can go upto 4GB per process as per my
understanding.
> thanks,
>
> greg k-h