| Message ID | 20240627060518.1510124-1-quic_ekangupt@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] misc: fastrpc: Remove user PD initmem size check | expand |
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 >
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
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
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 >>
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
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 >>> >
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
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;
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(-)