| Message ID | 20240530102032.27179-9-quic_ekangupt@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add missing features to FastRPC driver | expand |
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? > + * 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? > + > 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 >
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. > >> + * 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 >>
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? > > > > > > + * 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 > > >
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 >>>>
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 + * 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 (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; }
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(-)