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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4b90154d-2f03-48bc-95d7-6aa0aa57e9c9@quicinc.com>
Date: Fri, 28 Jun 2024 16:07:32 +0530
From: Ekansh Gupta <quic_ekangupt@...cinc.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <srinivas.kandagatla@...aro.org>, <linux-arm-msm@...r.kernel.org>,
        <quic_bkumar@...cinc.com>, <linux-kernel@...r.kernel.org>,
        <quic_chennak@...cinc.com>, <dri-devel@...ts.freedesktop.org>,
        <arnd@...db.de>, stable <stable@...nel.org>
Subject: Re: [PATCH v2] misc: fastrpc: Remove user PD initmem size check



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@...nel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.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