[PATCH V2 0/2] Fix SELinux denials against target driver

[PATCH V2 0/2] Fix SELinux denials against target driver

[Maurizio Lombardi]

Steps to reproduce:

1) install the ibacm, rdma-core and targetcli
2) service ibacm start   (ignore the errors)
3) Look at the dmesg, you will see an error message like
   "db_root: cannot open: /etc/target"

4) Execute $ sudo ausearch -m AVC,USER_AVC -ts recent

   type=AVC msg=audit(1707990698.893:610): avc:  denied  { read } for  pid=26447
   comm="systemd-modules" name="target" dev="dm-0" ino=973050 scontext=system_u:system_r:systemd_modules_load_t:s0
   tcontext=system_u:object_r:targetd_etc_rw_t:s0 tclass=dir permissive=0

Fix inspired by commit 581dd69830341d299b0c097fc366097ab497d679

V2: fix a memory leak in the error path, add a patch to set
    a freed pointer to NULL to avoid possible double frees

Maurizio Lombardi (2):
  target: fix selinux error when systemd-modules loads the target module
  target: set the xcopy_wq pointer to NULL after free.

 drivers/target/target_core_configfs.c | 12 ++++++++++++
 drivers/target/target_core_xcopy.c    |  4 +++-
 2 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.39.3

[PATCH V2 1/2] target: fix selinux error when systemd-modules loads the target module

[Maurizio Lombardi]

If the systemd-modules service loads the target module, the credentials
of that userspace process will be used to validate the access to the
target db directory.
selinux will prevent it, reporting an error like the following:

kernel: audit: type=1400 audit(1676301082.205:4): avc: denied  { read }
for  pid=1020 comm="systemd-modules" name="target" dev="dm-3"
ino=4657583 scontext=system_u:system_r:systemd_modules_load_t:s0
tcontext=system_u:object_r:targetd_etc_rw_t:s0 tclass=dir permissive=0

Fix the error by using the kernel credentials to access the db directory

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---

v2: call target_xcopy_release_pt() to destroy the xcopy_wq in case of failure

 drivers/target/target_core_configfs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index a5f58988130a..3f3dd0c0ce8c 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -3656,6 +3656,8 @@ static int __init target_core_init_configfs(void)
 {
 	struct configfs_subsystem *subsys = &target_core_fabrics;
 	struct t10_alua_lu_gp *lu_gp;
+	struct cred *kern_cred;
+	const struct cred *old_cred;
 	int ret;
 
 	pr_debug("TARGET_CORE[0]: Loading Generic Kernel Storage"
@@ -3732,11 +3734,21 @@ static int __init target_core_init_configfs(void)
 	if (ret < 0)
 		goto out;
 
+	/* We use the kernel credentials to access the target directory */
+	kern_cred = prepare_kernel_cred(&init_task);
+	if (!kern_cred) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	old_cred = override_creds(kern_cred);
 	target_init_dbroot();
+	revert_creds(old_cred);
+	put_cred(kern_cred);
 
 	return 0;
 
 out:
+	target_xcopy_release_pt();
 	configfs_unregister_subsystem(subsys);
 	core_dev_release_virtual_lun0();
 	rd_module_exit();
-- 
2.39.3

Re: [PATCH V2 1/2] target: fix selinux error when systemd-modules loads the target module

[michael.christie]

On 2/15/24 8:39 AM, Maurizio Lombardi wrote:
> If the systemd-modules service loads the target module, the credentials
> of that userspace process will be used to validate the access to the
> target db directory.
> selinux will prevent it, reporting an error like the following:
> 
> kernel: audit: type=1400 audit(1676301082.205:4): avc: denied  { read }
> for  pid=1020 comm="systemd-modules" name="target" dev="dm-3"
> ino=4657583 scontext=system_u:system_r:systemd_modules_load_t:s0
> tcontext=system_u:object_r:targetd_etc_rw_t:s0 tclass=dir permissive=0
> 
> Fix the error by using the kernel credentials to access the db directory
> 

Do you need something similar for the pr related dirs/files or how does
that work but not this?

Re: [PATCH V2 1/2] target: fix selinux error when systemd-modules loads the target module

[Maurizio Lombardi]

čt 15. 2. 2024 v 17:44 odesílatel <michael.christie@oracle.com> napsal:
> Do you need something similar for the pr related dirs/files or how does
> that work but not this?
>

I think that in that case it won't be necessary because the pr code is executed
by a kernel thread that calls the execute_cmd() callback, not by a
user process in kernel context,
but I will try and eventually I will report back the findings

Maurizio

Re: [PATCH V2 1/2] target: fix selinux error when systemd-modules loads the target module

[Mike Christie]

On 2/15/24 11:07 AM, Maurizio Lombardi wrote:
> čt 15. 2. 2024 v 17:44 odesílatel <michael.christie@oracle.com> napsal:
>> Do you need something similar for the pr related dirs/files or how does
>> that work but not this?
>>
> 
> I think that in that case it won't be necessary because the pr code is executed
> by a kernel thread that calls the execute_cmd() callback, not by a
> user process in kernel context,
> but I will try and eventually I will report back the findings
> 

Ignore my comment. I was thinking of something completely different and
figured out I was wrong when I looked into it.

The target parts look ok:

Reviewed-by: Mike Christie <michael.christie@oracle.com>

I have no idea about the creds part. If the patch:

commit 581dd69830341d299b0c097fc366097ab497d679
Author: Thiébaud Weksteen <tweek@google.com>
Date:   Mon May 2 10:49:52 2022 +1000

    firmware_loader: use kernel credentials when reading firmware

you referenced is the correct way to do it, then it looks ok.

[PATCH V2 2/2] target: set the xcopy_wq pointer to NULL after free.

[Maurizio Lombardi]

Do not leave a dangling pointer after free.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/target/target_core_xcopy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 4128631c9dfd..1f79da0041e3 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -491,8 +491,10 @@ int target_xcopy_setup_pt(void)
 
 void target_xcopy_release_pt(void)
 {
-	if (xcopy_wq)
+	if (xcopy_wq) {
 		destroy_workqueue(xcopy_wq);
+		xcopy_wq = NULL;
+	}
 }
 
 /*
-- 
2.39.3

Re: [PATCH V2 2/2] target: set the xcopy_wq pointer to NULL after free.

[michael.christie]

On 2/15/24 8:39 AM, Maurizio Lombardi wrote:
> Do not leave a dangling pointer after free.
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/target/target_core_xcopy.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
> index 4128631c9dfd..1f79da0041e3 100644
> --- a/drivers/target/target_core_xcopy.c
> +++ b/drivers/target/target_core_xcopy.c
> @@ -491,8 +491,10 @@ int target_xcopy_setup_pt(void)
>  
>  void target_xcopy_release_pt(void)
>  {
> -	if (xcopy_wq)
> +	if (xcopy_wq) {
>  		destroy_workqueue(xcopy_wq);
> +		xcopy_wq = NULL;
> +	}
>  }
>  

Why do you need this? Isn't this only called when the module is unloaded?

We don't normally do this for that type of case in general. In the target
code alone we have lots of places we don't do this.

Re: [PATCH V2 2/2] target: set the xcopy_wq pointer to NULL after free.

[Maurizio Lombardi]

čt 15. 2. 2024 v 17:42 odesílatel <michael.christie@oracle.com> napsal:
> Why do you need this? Isn't this only called when the module is unloaded?
>
> We don't normally do this for that type of case in general. In the target
> code alone we have lots of places we don't do this.
>

I don't really need this, it's just a matter of personal preference,
we can drop this one.

Maurizio

Re: (subset) [PATCH V2 0/2] Fix SELinux denials against target driver

[Martin K. Petersen]

On Thu, 15 Feb 2024 15:39:42 +0100, Maurizio Lombardi wrote:

> Steps to reproduce:
> 
> 1) install the ibacm, rdma-core and targetcli
> 2) service ibacm start   (ignore the errors)
> 3) Look at the dmesg, you will see an error message like
>    "db_root: cannot open: /etc/target"
> 
> [...]

Applied to 6.9/scsi-fixes, thanks!

[1/2] target: fix selinux error when systemd-modules loads the target module
      https://git.kernel.org/mkp/scsi/c/97a54ef596c3

-- 
Martin K. Petersen	Oracle Linux Engineering