| Message ID | 20240530102032.27179-1-quic_ekangupt@quicinc.com |
|---|---|
| Headers | show |
| Series | Add missing features to FastRPC driver | expand |
On 30/05/2024 11:20, Ekansh Gupta wrote: > DSP capabilities request is sending bad size to utilities skel What you exactly mean by this? Curretly driver is sending 1024 bytes of buffer, why is DSP not happy with this size? > call which is resulting in memory corruption. Pass proper size What does proper size mean? > to avoid the corruption. > > 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 | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 61389795f498..3e1ab58038ed 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1695,6 +1695,7 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr > > /* Capability filled in userspace */ > dsp_attr_buf[0] = 0; > + dsp_attr_buf_len -= 1; is DSP expecting 255 *4 bytes instead of 256 *4? --srini > > args[0].ptr = (u64)(uintptr_t)&dsp_attr_buf_len; > args[0].length = sizeof(dsp_attr_buf_len);
On 30/05/2024 11:20, Ekansh Gupta wrote: > Untrusted application with access to only non-secure fastrpc device > node can attach to root_pd or static PDs if it can make the respective > init request. This can cause problems as the untrusted application > can send bad requests to root_pd or static PDs. Add changes to reject > attach to privileged PDs if the request is being made using non-secure > fastrpc device node. > > Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd") > Cc: stable <stable@kernel.org> > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > drivers/misc/fastrpc.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index d9d9f889e39e..73fa0e536cf9 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1344,6 +1344,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > } inbuf; > u32 sc; > > + if (!fl->is_secure_dev) { > + dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n"); > + return -EACCES; > + } Please move these checks to fastrpc_device_ioctl which makes it clear that these are only supported with secure device nodes. I would also prefer this to be documented in the the uapi headers. --srini > + > args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL); > if (!args) > return -ENOMEM; > @@ -1769,6 +1774,11 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd) > int tgid = fl->tgid; > u32 sc; > > + if (!fl->is_secure_dev) { > + dev_dbg(&fl->cctx->rpdev->dev, "untrusted app trying to attach to privileged DSP PD\n"); > + return -EACCES; > + } > + > args[0].ptr = (u64)(uintptr_t) &tgid; > args[0].length = sizeof(tgid); > args[0].fd = -1;
On 6/3/2024 3:32 PM, Dmitry Baryshkov wrote: > On Mon, Jun 03, 2024 at 11:57:52AM +0530, Ekansh Gupta wrote: >> On 5/31/2024 5:19 AM, Dmitry Baryshkov wrote: >>> On Thu, May 30, 2024 at 03:50:26PM +0530, Ekansh Gupta wrote: >>>> Some untrusted applications will not have access to open fastrpc >>>> device nodes and a privileged process can open the device node on >>>> behalf of the application. Add a check to restrict such untrusted >>>> applications from offloading to signed PD. >>>> >>>> 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> >>>> --- >>>> drivers/misc/fastrpc.c | 23 ++++++++++++++++++----- >>>> 1 file changed, 18 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >>>> index 73fa0e536cf9..32615ccde7ac 100644 >>>> --- a/drivers/misc/fastrpc.c >>>> +++ b/drivers/misc/fastrpc.c >>>> @@ -328,6 +328,7 @@ struct fastrpc_user { >>>> int pd; >>>> bool is_secure_dev; >>>> bool is_unsigned_pd; >>>> + bool untrusted_process; >>>> char *servloc_name; >>>> /* Lock for lists */ >>>> spinlock_t lock; >>>> @@ -1249,13 +1250,17 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques >>>> * channel is configured as secure and block untrusted apps on channel >>>> * that does not support unsigned PD offload >>>> */ >>>> - if (!fl->cctx->unsigned_support || !unsigned_pd_request) { >>>> - dev_err(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n"); >>>> - return true; >>>> - } >>>> + if (!fl->cctx->unsigned_support || !unsigned_pd_request) >>>> + goto reject_session; >>>> } >>>> + /* Check if untrusted process is trying to offload to signed PD */ >>>> + if (fl->untrusted_process && !unsigned_pd_request) >>>> + goto reject_session; >>>> return false; >>>> +reject_session: >>>> + dev_dbg(&fl->cctx->rpdev->dev, "Error: Untrusted application trying to offload to signed PD\n"); >>>> + return true; >>>> } >>>> static void fastrpc_mmap_remove_pdr(struct fastrpc_static_pd *spd) >>>> @@ -1504,12 +1509,20 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl, >>>> goto err; >>>> } >>>> + /* >>>> + * Third-party apps don't have permission to open the fastrpc device, so >>> Permissions depend on the end-user setup. Is it going to break if the >>> user sets 0666 mode for fastrpc nodes? >> If the root user sets 0666 for fastrpc nodes, it is expected that this check will get bypassed. > So, any process will be trusted? This looks so Android-centric. Please come > with a better way to define 'trusted'. > > On a typical UNIX system a used has multiple supplementary GIDs (which > can be used to allow access to the devices) which have no relationship > to the process effective GID. On a multi-user machine it might be > logical that fastrpc nodes have separate group-id and group's read/write > permissions. But then each of the users has their own unique 'effective' > GID. Which of those should be using for computing the 'trusted' status? Thanks for your suggestions, Dmitry. I am considering dropping this patch and system unsignedPD patch from this series(due to the dependency). I'm redesigning the trusted-process term to make it more generic. Planning to make it depend on the group IDs and have a check with both primary and supplementary GIDs of the process. I'll share the design with you along with the changes once it's ready. > >>>> + * it is opened on their behalf by a priveleged process. This is detected >>>> + * by comparing current PID with the one stored during device open. >>>> + */ >>>> + if (current->tgid != fl->tgid) >>>> + fl->untrusted_process = true; >>> If the comment talks about PIDs, when why are you comparing GIDs here? >> It should be GID, I'll update the comment in next spin. >> >>>> + >>>> if (init.attrs & FASTRPC_MODE_UNSIGNED_MODULE) >>>> fl->is_unsigned_pd = true; >>>> if (is_session_rejected(fl, fl->is_unsigned_pd)) { >>>> - err = -ECONNREFUSED; >>>> + err = -EACCES; >>>> goto err; >>>> } >>>> -- >>>> 2.43.0 >>>>