On Tue, Jan 31, 2023 at 01:51:39PM +0100, Roberto Sassu wrote:

On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote:

quoted

+
+/**
+ * ipe_mmap_file - ipe security hook function for mmap check.
+ * @f: File being mmap'd. Can be NULL in the case of anonymous memory.
+ * @reqprot: The requested protection on the mmap, passed from usermode.
+ * @prot: The effective protection on the mmap, resolved from reqprot and
+ *	  system configuration.
+ * @flags: Unused.
+ *
+ * This hook is called when a file is loaded through the mmap
+ * family of system calls.
+ *
+ * Return:
+ * * 0	- OK
+ * * !0	- Error
+ */
+int ipe_mmap_file(struct file *f, unsigned long reqprot, unsigned long prot,
+		  unsigned long flags)
+{
+	struct ipe_eval_ctx ctx = { 0 };
+
+	if (prot & PROT_EXEC || reqprot & PROT_EXEC) {

Since the kernel only adds flags and doesn't clear them, isn't safe to
just consider prot? Oh, you mentioned it in the changelog, maybe just
for ipe_file_mprotect().

Thanks for pointing that out, yes reqprot it indeed unnecessary, I will remove
this part in the next version. 

quoted

+		build_eval_ctx(&ctx, f, ipe_op_exec);
+		return ipe_evaluate_event(&ctx);
+	}

Uhm, I think some considerations that IMA does for mmap() are relevant
also for IPE.

For example, look at mmap_violation_check(). It checks if there are
writable mappings, and if yes, it denies the access.

Similarly for mprotect(), is adding PROT_EXEC safe?

Yes, writable mapping might need to treat differently. But for the current version
I think it is safe because currently we only support dmverity and fsverity,
they are inherently read-only.

But if in the future if there is a feature can support writable mapping, IPE might
better provide user the flexibility to allow or deny execute writable mappings,
for example, adding a new property like file_writable=TRUE. Then user can deploy
a rule like op=EXECUTE file_writable=TRUE action=DENY to deny execute a writable
mapping.

quoted

@@ -12,6 +13,11 @@ static struct lsm_blob_sizes ipe_blobs __lsm_ro_after_init = {
 
 static struct security_hook_list ipe_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sb_free_security, ipe_sb_free_security),
+	LSM_HOOK_INIT(bprm_check_security, ipe_bprm_check_security),
+	LSM_HOOK_INIT(mmap_file, ipe_mmap_file),
+	LSM_HOOK_INIT(file_mprotect, ipe_file_mprotect),
+	LSM_HOOK_INIT(kernel_read_file, ipe_kernel_read_file),
+	LSM_HOOK_INIT(kernel_load_data, ipe_kernel_load_data),
 };

Uhm, maybe I would incorporate patch 1 with this.

Roberto

This might not be possible because this patch has some dependencies on the previous patches.
-Fan