Date:   Thu, 9 Feb 2023 14:42:43 -0800
From:   Fan Wu <wufan@linux.microsoft.com>
To:     Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc:     corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
        serge@hallyn.com, tytso@mit.edu, ebiggers@kernel.org,
        axboe@kernel.dk, agk@redhat.com, snitzer@kernel.org,
        eparis@redhat.com, paul@paul-moore.com, linux-doc@vger.kernel.org,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-block@vger.kernel.org,
        dm-devel@redhat.com, linux-audit@redhat.com,
        roberto.sassu@huawei.com, linux-kernel@vger.kernel.org,
        Deven Bowers <deven.desai@linux.microsoft.com>
Subject: Re: [RFC PATCH v9 06/16] ipe: add LSM hooks on execution and kernel
 read
Message-ID: <20230209224243.GA9462@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
References: <1675119451-23180-1-git-send-email-wufan@linux.microsoft.com>
 <1675119451-23180-7-git-send-email-wufan@linux.microsoft.com>
 <412da9a9da2e75e896911f01bfd735dd4b5789f4.camel@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <412da9a9da2e75e896911f01bfd735dd4b5789f4.camel@huaweicloud.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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:
> > +
> > +/**
> > + * 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. 

> > +		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.

> >  
> > @@ -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