[PATCH 1/4] module: Do not access sig_enforce directly

[PATCH 1/4] module: Do not access sig_enforce directly

[Jia Zhang]

Call is_module_sig_enforced() instead.

Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index ad2d420..003d0ab 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2789,7 +2789,7 @@ static int module_sig_check(struct load_info *info, int flags)
 	}
 
 	/* Not having a signature is only an error if we're strict. */
-	if (err == -ENOKEY && !sig_enforce)
+	if (err == -ENOKEY && !is_module_sig_enforced())
 		err = 0;
 
 	return err;
-- 
1.8.3.1

[PATCH 2/4] module: Create the entry point initialize_module()

[Jia Zhang]

This entry point currently includes the procfs initialization,
and will include a securityfs initialization.

Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
---
 kernel/module.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 003d0ab..79825ea 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4243,7 +4243,11 @@ static int __init proc_modules_init(void)
 	proc_create("modules", 0, NULL, &proc_modules_operations);
 	return 0;
 }
-module_init(proc_modules_init);
+#else	/* CONFIG_PROC_FS */
+static int __init proc_modules_init(void)
+{
+        return 0;
+}
 #endif
 
 /* Given an address, look for it in the module exception tables. */
@@ -4388,3 +4392,11 @@ void module_layout(struct module *mod,
 }
 EXPORT_SYMBOL(module_layout);
 #endif
+
+static int __init initialize_module(void)
+{
+	proc_modules_init();
+
+	return 0;
+}
+module_init(initialize_module);
-- 
1.8.3.1

[PATCH 3/4] module: Support to show the current enforcement policy

[Jia Zhang]

/sys/kernel/security/modsign/enforce gives the result of current
enforcement policy of loading module.

Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
---
 kernel/module.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/kernel/module.c b/kernel/module.c
index 79825ea..e3c6c8e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2794,11 +2794,60 @@ static int module_sig_check(struct load_info *info, int flags)
 
 	return err;
 }
+
+#ifdef CONFIG_SECURITYFS
+static ssize_t modsign_enforce_read(struct file *filp, char __user *ubuf,
+				    size_t count, loff_t *offp)
+{
+	char buf[2];
+
+	sprintf(buf, "%d", !!sig_enforce);
+
+	return simple_read_from_buffer(ubuf, count, offp, buf, 1);
+}
+
+static const struct file_operations modsign_enforce_ops = {
+	.read = modsign_enforce_read,
+	.llseek = generic_file_llseek,
+};
+
+static int __init securityfs_init(void)
+{
+	struct dentry *modsign_dir;
+	struct dentry *enforce;
+
+	modsign_dir = securityfs_create_dir("modsign", NULL);
+	if (IS_ERR(modsign_dir))
+		return -1;
+
+	enforce = securityfs_create_file("enforce",
+					 S_IRUSR | S_IRGRP, modsign_dir,
+					 NULL, &modsign_enforce_ops);
+	if (IS_ERR(enforce))
+		goto out;
+
+	return 0;
+out:
+	securityfs_remove(modsign_dir);
+
+	return -1;
+}
+#else /* !CONFIG_SECURITYFS */
+static int __init securityfs_init(void)
+{
+	return 0;
+}
+#endif
 #else /* !CONFIG_MODULE_SIG */
 static int module_sig_check(struct load_info *info, int flags)
 {
 	return 0;
 }
+
+static int __init securityfs_init(void)
+{
+	return 0;
+}
 #endif /* !CONFIG_MODULE_SIG */
 
 /* Sanity checks against invalid binaries, wrong arch, weird elf version. */
@@ -4395,8 +4444,14 @@ void module_layout(struct module *mod,
 
 static int __init initialize_module(void)
 {
+	int ret;
+
 	proc_modules_init();
 
+	ret = securityfs_init();
+	if (unlikely(ret))
+		return ret;
+
 	return 0;
 }
 module_init(initialize_module);
-- 
1.8.3.1

[PATCH 4/4] module: Allow to upgrade to validity enforcement in unforced mode

[Jia Zhang]

If module signature verification check is enabled but the
validity enforcement is configured to be disabled, it should
be allowed to enable it. Once enabled, it is disallowed to
disable it.

Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
---
 kernel/module.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index e3c6c8e..89704df 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2806,8 +2806,37 @@ static ssize_t modsign_enforce_read(struct file *filp, char __user *ubuf,
 	return simple_read_from_buffer(ubuf, count, offp, buf, 1);
 }
 
+#ifndef CONFIG_MODULE_SIG_FORCE
+static ssize_t modsign_enforce_write(struct file *filp,
+				     const char __user *ubuf,
+				     size_t count, loff_t *offp)
+{
+	char buf;
+	ssize_t ret;
+
+	if (*offp > 1)
+		return -EFBIG;
+
+	ret = simple_write_to_buffer(&buf, 1, offp, ubuf, count);
+	if (ret > 0) {
+		if (buf != '1')
+			return -EINVAL;
+
+		sig_enforce = true;
+		pr_notice_once("Kernel module validity enforcement enabled\n");
+
+		ret = count;
+	}
+
+	return ret;
+}
+#endif
+
 static const struct file_operations modsign_enforce_ops = {
 	.read = modsign_enforce_read,
+#ifndef CONFIG_MODULE_SIG_FORCE
+	.write = modsign_enforce_write,
+#endif
 	.llseek = generic_file_llseek,
 };
 
@@ -2815,14 +2844,18 @@ static int __init securityfs_init(void)
 {
 	struct dentry *modsign_dir;
 	struct dentry *enforce;
+	umode_t mode;
 
 	modsign_dir = securityfs_create_dir("modsign", NULL);
 	if (IS_ERR(modsign_dir))
 		return -1;
 
-	enforce = securityfs_create_file("enforce",
-					 S_IRUSR | S_IRGRP, modsign_dir,
-					 NULL, &modsign_enforce_ops);
+	mode = S_IRUSR | S_IRGRP;
+	if (!sig_enforce)
+		mode |= S_IWUSR;
+
+	enforce = securityfs_create_file("enforce", mode, modsign_dir, NULL,
+					 &modsign_enforce_ops);
 	if (IS_ERR(enforce))
 		goto out;
 
-- 
1.8.3.1

Re: [PATCH 1/4] module: Do not access sig_enforce directly

[Jia Zhang]

Hi Jessica,

Could you review this patch series?

Thanks,
Jia

On 2018/3/1 下午5:09, Jia Zhang wrote:
> Call is_module_sig_enforced() instead.
> 
> Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
> ---
>  kernel/module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index ad2d420..003d0ab 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2789,7 +2789,7 @@ static int module_sig_check(struct load_info *info, int flags)
>  	}
>  
>  	/* Not having a signature is only an error if we're strict. */
> -	if (err == -ENOKEY && !sig_enforce)
> +	if (err == -ENOKEY && !is_module_sig_enforced())
>  		err = 0;
>  
>  	return err;
> 

Re: [PATCH 3/4] module: Support to show the current enforcement policy

[Jessica Yu]

+++ Jia Zhang [01/03/18 17:09 +0800]:
>/sys/kernel/security/modsign/enforce gives the result of current
>enforcement policy of loading module.
>
>Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>

Why is this being added as part of securityfs? AFAIK that's primarily used by LSMs.

And we already export sig_enforce to sysfs (See /sys/module/module/parameters/sig_enforce).
It already does exactly what your patchset tries to do, it only allows for enablement. 

Jessica

>---
> kernel/module.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index 79825ea..e3c6c8e 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2794,11 +2794,60 @@ static int module_sig_check(struct load_info *info, int flags)
>
> 	return err;
> }
>+
>+#ifdef CONFIG_SECURITYFS
>+static ssize_t modsign_enforce_read(struct file *filp, char __user *ubuf,
>+				    size_t count, loff_t *offp)
>+{
>+	char buf[2];
>+
>+	sprintf(buf, "%d", !!sig_enforce);
>+
>+	return simple_read_from_buffer(ubuf, count, offp, buf, 1);
>+}
>+
>+static const struct file_operations modsign_enforce_ops = {
>+	.read = modsign_enforce_read,
>+	.llseek = generic_file_llseek,
>+};
>+
>+static int __init securityfs_init(void)
>+{
>+	struct dentry *modsign_dir;
>+	struct dentry *enforce;
>+
>+	modsign_dir = securityfs_create_dir("modsign", NULL);
>+	if (IS_ERR(modsign_dir))
>+		return -1;
>+
>+	enforce = securityfs_create_file("enforce",
>+					 S_IRUSR | S_IRGRP, modsign_dir,
>+					 NULL, &modsign_enforce_ops);
>+	if (IS_ERR(enforce))
>+		goto out;
>+
>+	return 0;
>+out:
>+	securityfs_remove(modsign_dir);
>+
>+	return -1;
>+}
>+#else /* !CONFIG_SECURITYFS */
>+static int __init securityfs_init(void)
>+{
>+	return 0;
>+}
>+#endif
> #else /* !CONFIG_MODULE_SIG */
> static int module_sig_check(struct load_info *info, int flags)
> {
> 	return 0;
> }
>+
>+static int __init securityfs_init(void)
>+{
>+	return 0;
>+}
> #endif /* !CONFIG_MODULE_SIG */
>
> /* Sanity checks against invalid binaries, wrong arch, weird elf version. */
>@@ -4395,8 +4444,14 @@ void module_layout(struct module *mod,
>
> static int __init initialize_module(void)
> {
>+	int ret;
>+
> 	proc_modules_init();
>
>+	ret = securityfs_init();
>+	if (unlikely(ret))
>+		return ret;
>+
> 	return 0;
> }
> module_init(initialize_module);
>-- 
>1.8.3.1
>

Re: [PATCH 3/4] module: Support to show the current enforcement policy

[Jia Zhang]

On 2018/3/8 上午4:14, Jessica Yu wrote:
> +++ Jia Zhang [01/03/18 17:09 +0800]:
>> /sys/kernel/security/modsign/enforce gives the result of current
>> enforcement policy of loading module.
>>
>> Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
> 
> Why is this being added as part of securityfs? AFAIK that's primarily
> used by LSMs.

The integrity subsystem such as IMA is also located there.

> 
> And we already export sig_enforce to sysfs (See
> /sys/module/module/parameters/sig_enforce).
> It already does exactly what your patchset tries to do, it only allows
> for enablement.

I will respond this in V2.

Thanks,
Jia

> Jessica
> 
>> ---
>> kernel/module.c | 55
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 79825ea..e3c6c8e 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2794,11 +2794,60 @@ static int module_sig_check(struct load_info
>> *info, int flags)
>>
>>     return err;
>> }
>> +
>> +#ifdef CONFIG_SECURITYFS
>> +static ssize_t modsign_enforce_read(struct file *filp, char __user
>> *ubuf,
>> +                    size_t count, loff_t *offp)
>> +{
>> +    char buf[2];
>> +
>> +    sprintf(buf, "%d", !!sig_enforce);
>> +
>> +    return simple_read_from_buffer(ubuf, count, offp, buf, 1);
>> +}
>> +
>> +static const struct file_operations modsign_enforce_ops = {
>> +    .read = modsign_enforce_read,
>> +    .llseek = generic_file_llseek,
>> +};
>> +
>> +static int __init securityfs_init(void)
>> +{
>> +    struct dentry *modsign_dir;
>> +    struct dentry *enforce;
>> +
>> +    modsign_dir = securityfs_create_dir("modsign", NULL);
>> +    if (IS_ERR(modsign_dir))
>> +        return -1;
>> +
>> +    enforce = securityfs_create_file("enforce",
>> +                     S_IRUSR | S_IRGRP, modsign_dir,
>> +                     NULL, &modsign_enforce_ops);
>> +    if (IS_ERR(enforce))
>> +        goto out;
>> +
>> +    return 0;
>> +out:
>> +    securityfs_remove(modsign_dir);
>> +
>> +    return -1;
>> +}
>> +#else /* !CONFIG_SECURITYFS */
>> +static int __init securityfs_init(void)
>> +{
>> +    return 0;
>> +}
>> +#endif
>> #else /* !CONFIG_MODULE_SIG */
>> static int module_sig_check(struct load_info *info, int flags)
>> {
>>     return 0;
>> }
>> +
>> +static int __init securityfs_init(void)
>> +{
>> +    return 0;
>> +}
>> #endif /* !CONFIG_MODULE_SIG */
>>
>> /* Sanity checks against invalid binaries, wrong arch, weird elf
>> version. */
>> @@ -4395,8 +4444,14 @@ void module_layout(struct module *mod,
>>
>> static int __init initialize_module(void)
>> {
>> +    int ret;
>> +
>>     proc_modules_init();
>>
>> +    ret = securityfs_init();
>> +    if (unlikely(ret))
>> +        return ret;
>> +
>>     return 0;
>> }
>> module_init(initialize_module);
>> -- 
>> 1.8.3.1
>>

[PATCH 1/4] module: Do not access sig_enforce directly

[Jia Zhang]

Call is_module_sig_enforced() instead.

Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index ad2d420..003d0ab 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2789,7 +2789,7 @@ static int module_sig_check(struct load_info *info, int flags)
 	}
 
 	/* Not having a signature is only an error if we're strict. */
-	if (err == -ENOKEY && !sig_enforce)
+	if (err == -ENOKEY && !is_module_sig_enforced())
 		err = 0;
 
 	return err;
-- 
1.8.3.1