Linux Security Modules development
 help / color / mirror / Atom feed

* [PATCH v15 3/4] overlayfs: internal getxattr operations without sepolicy checking
From: Mark Salyzyn @ 2019-11-04 21:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: kernel-team, Mark Salyzyn, Miklos Szeredi, Jonathan Corbet,
	Vivek Goyal, Eric W . Biederman, Amir Goldstein, Randy Dunlap,
	Stephen Smalley, linux-unionfs, linux-doc, linux-security-module
In-Reply-To: <20191104215253.141818-1-salyzyn@android.com>

Check impure, opaque, origin & meta xattr with no sepolicy audit
(using __vfs_getxattr) since these operations are internal to
overlayfs operations and do not disclose any data.  This became
an issue for credential override off since sys_admin would have
been required by the caller; whereas would have been inherently
present for the creator since it performed the mount.

This is a change in operations since we do not check in the new
ovl_do_vfs_getxattr function if the credential override is off or
not.  Reasoning is that the sepolicy check is unnecessary overhead,
especially since the check can be expensive.

Because for override credentials off, this affects _everyone_ that
underneath performs private xattr calls without the appropriate
sepolicy permissions and sys_admin capability.  Providing blanket
support for sys_admin would be bad for all possible callers.

For the override credentials on, this will affect only the mounter,
should it lack sepolicy permissions. Not considered a security
problem since mounting by definition has sys_admin capabilities,
but sepolicy contexts would still need to be crafted.

It should be noted that there is precedence, __vfs_getxattr is used
in other filesystems for their own internal trusted xattr management.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: linux-security-module@vger.kernel.org

v15 - revert to v13 as xattr_gs_args was rejected.
    - move ovl_do_wrapper from util.c to inline in overlayfs.h

v14 - rebase to use xattr_gs_args.

v13 - rebase to use __vfs_getxattr flags option

v12 - rebase

v11 - switch name to ovl_do_vfs_getxattr, fortify comment

v10 - added to patch series
---
 fs/overlayfs/namei.c     | 12 +++++++-----
 fs/overlayfs/overlayfs.h |  8 ++++++++
 fs/overlayfs/util.c      | 18 +++++++++---------
 3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e9717c2f7d45..f5aba0a0767b 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -106,10 +106,11 @@ int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
 
 static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
 {
-	int res, err;
+	ssize_t res;
+	int err;
 	struct ovl_fh *fh = NULL;
 
-	res = vfs_getxattr(dentry, name, NULL, 0);
+	res = ovl_do_vfs_getxattr(dentry, name, NULL, 0);
 	if (res < 0) {
 		if (res == -ENODATA || res == -EOPNOTSUPP)
 			return NULL;
@@ -123,7 +124,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
 	if (!fh)
 		return ERR_PTR(-ENOMEM);
 
-	res = vfs_getxattr(dentry, name, fh, res);
+	res = ovl_do_vfs_getxattr(dentry, name, fh, res);
 	if (res < 0)
 		goto fail;
 
@@ -141,10 +142,11 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
 	return NULL;
 
 fail:
-	pr_warn_ratelimited("overlayfs: failed to get origin (%i)\n", res);
+	pr_warn_ratelimited("overlayfs: failed to get origin (%zi)\n", res);
 	goto out;
 invalid:
-	pr_warn_ratelimited("overlayfs: invalid origin (%*phN)\n", res, fh);
+	pr_warn_ratelimited("overlayfs: invalid origin (%*phN)\n",
+			    (int)res, fh);
 	goto out;
 }
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index ab3d031c422b..55b872c28bf9 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -200,6 +200,14 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
 	return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
 }
 
+static inline ssize_t ovl_do_vfs_getxattr(struct dentry *dentry,
+					  const char *name, void *buf,
+					  size_t size)
+{
+	return __vfs_getxattr(dentry, d_inode(dentry), name, buf, size,
+			      XATTR_NOSECURITY);
+}
+
 /* util.c */
 int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f5678a3f8350..2050c5084a82 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -537,9 +537,9 @@ void ovl_copy_up_end(struct dentry *dentry)
 
 bool ovl_check_origin_xattr(struct dentry *dentry)
 {
-	int res;
+	ssize_t res;
 
-	res = vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
+	res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
 
 	/* Zero size value means "copied up but origin unknown" */
 	if (res >= 0)
@@ -550,13 +550,13 @@ bool ovl_check_origin_xattr(struct dentry *dentry)
 
 bool ovl_check_dir_xattr(struct dentry *dentry, const char *name)
 {
-	int res;
+	ssize_t res;
 	char val;
 
 	if (!d_is_dir(dentry))
 		return false;
 
-	res = vfs_getxattr(dentry, name, &val, 1);
+	res = ovl_do_vfs_getxattr(dentry, name, &val, 1);
 	if (res == 1 && val == 'y')
 		return true;
 
@@ -837,13 +837,13 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
 /* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
 int ovl_check_metacopy_xattr(struct dentry *dentry)
 {
-	int res;
+	ssize_t res;
 
 	/* Only regular files can have metacopy xattr */
 	if (!S_ISREG(d_inode(dentry)->i_mode))
 		return 0;
 
-	res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+	res = ovl_do_vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
 	if (res < 0) {
 		if (res == -ENODATA || res == -EOPNOTSUPP)
 			return 0;
@@ -852,7 +852,7 @@ int ovl_check_metacopy_xattr(struct dentry *dentry)
 
 	return 1;
 out:
-	pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+	pr_warn_ratelimited("overlayfs: failed to get metacopy (%zi)\n", res);
 	return res;
 }
 
@@ -878,7 +878,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
 	ssize_t res;
 	char *buf = NULL;
 
-	res = vfs_getxattr(dentry, name, NULL, 0);
+	res = ovl_do_vfs_getxattr(dentry, name, NULL, 0);
 	if (res < 0) {
 		if (res == -ENODATA || res == -EOPNOTSUPP)
 			return -ENODATA;
@@ -890,7 +890,7 @@ ssize_t ovl_getxattr(struct dentry *dentry, char *name, char **value,
 		if (!buf)
 			return -ENOMEM;
 
-		res = vfs_getxattr(dentry, name, buf, res);
+		res = ovl_do_vfs_getxattr(dentry, name, buf, res);
 		if (res < 0)
 			goto fail;
 	}
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply related

* [PATCH v15 2/4] overlayfs: handle XATTR_NOSECURITY flag for get xattr method
From: Mark Salyzyn @ 2019-11-04 21:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: kernel-team, Mark Salyzyn, Miklos Szeredi, Jonathan Corbet,
	Vivek Goyal, Eric W . Biederman, Amir Goldstein, Randy Dunlap,
	Stephen Smalley, linux-unionfs, linux-doc, linux-security-module
In-Reply-To: <20191104215253.141818-1-salyzyn@android.com>

Because of the overlayfs getxattr recursion, the incoming inode fails
to update the selinux sid resulting in avc denials being reported
against a target context of u:object_r:unlabeled:s0.

Solution is to respond to the XATTR_NOSECURITY flag in get xattr
method that calls the __vfs_getxattr handler instead so that the
context can be read in, rather than being denied with an -EACCES
when vfs_getxattr handler is called.

For the use case where access is to be blocked by the security layer.

The path then would be security(dentry) ->
__vfs_getxattr({dentry...XATTR_NOSECURITY}) ->
handler->get({dentry...XATTR_NOSECURITY}) ->
__vfs_getxattr({realdentry...XATTR_NOSECURITY}) ->
lower_handler->get({realdentry...XATTR_NOSECURITY}) which
would report back through the chain data and success as expected,
the logging security layer at the top would have the data to
determine the access permissions and report back to the logs and
the caller that the target context was blocked.

For selinux this would solve the cosmetic issue of the selinux log
and allow audit2allow to correctly report the rule needed to address
the access problem.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: linux-unionfs@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: linux-security-module@vger.kernel.org

v15 - revert to v13 because xattr_gs_args rejected.

v14 - rebase to use xattr_gs_args.

v13 - rebase to use __vfs_getxattr flags option.

v12 - Added back to patch series as get xattr with flag option.

v11 - Squashed out of patch series and replaced with per-thread flag
      solution.

v10 - Added to patch series as __get xattr method.
---
 fs/overlayfs/inode.c     | 5 +++--
 fs/overlayfs/overlayfs.h | 2 +-
 fs/overlayfs/super.c     | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index bc14781886bf..c057e51057f7 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -363,7 +363,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
 }
 
 int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
-		  void *value, size_t size)
+		  void *value, size_t size, int flags)
 {
 	ssize_t res;
 	const struct cred *old_cred;
@@ -371,7 +371,8 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
 		ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
 
 	old_cred = ovl_override_creds(dentry->d_sb);
-	res = vfs_getxattr(realdentry, name, value, size);
+	res = __vfs_getxattr(realdentry, d_inode(realdentry), name,
+			     value, size, flags);
 	revert_creds(old_cred);
 	return res;
 }
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6934bcf030f0..ab3d031c422b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -356,7 +356,7 @@ int ovl_permission(struct inode *inode, int mask);
 int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
 		  const void *value, size_t size, int flags);
 int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
-		  void *value, size_t size);
+		  void *value, size_t size, int flags);
 ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
 struct posix_acl *ovl_get_acl(struct inode *inode, int type);
 int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 57f5f948ae0a..c91e7b604631 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -857,7 +857,7 @@ ovl_posix_acl_xattr_get(const struct xattr_handler *handler,
 			struct dentry *dentry, struct inode *inode,
 			const char *name, void *buffer, size_t size, int flags)
 {
-	return ovl_xattr_get(dentry, inode, handler->name, buffer, size);
+	return ovl_xattr_get(dentry, inode, handler->name, buffer, size, flags);
 }
 
 static int __maybe_unused
@@ -939,7 +939,7 @@ static int ovl_other_xattr_get(const struct xattr_handler *handler,
 			       const char *name, void *buffer, size_t size,
 			       int flags)
 {
-	return ovl_xattr_get(dentry, inode, name, buffer, size);
+	return ovl_xattr_get(dentry, inode, name, buffer, size, flags);
 }
 
 static int ovl_other_xattr_set(const struct xattr_handler *handler,
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply related

* Re: [PATCH v14 1/5] Add flags option to get xattr method paired to __vfs_getxattr
From: Mark Salyzyn @ 2019-11-04 21:51 UTC (permalink / raw)
  To: Amir Goldstein, Andreas Dilger
  Cc: Greg Kroah-Hartman, Linux Kernel Mailing List, linux-doc, CIFS,
	kernel-team, selinux, Linux FS-devel Mailing List,
	Jonathan Corbet, Ext4 Developers List, Stephen Smalley, overlayfs,
	Andreas Gruenbacher, ecryptfs, LSM List, Alexander Viro,
	Christoph Hellwig
In-Reply-To: <CAOQ4uxis-oQSjKrtBDi-8BQ2M3ve3w8o-YVGRwWLnq+5JLUttA@mail.gmail.com>

On 10/23/19 9:57 PM, Amir Goldstein wrote:
> [excessive CC list reduced]
>
> On Wed, Oct 23, 2019 at 11:07 AM Andreas Dilger via samba-technical
> <samba-technical@lists.samba.org> wrote:
>>
>> On Oct 22, 2019, at 2:44 PM, Mark Salyzyn <salyzyn@android.com> wrote:
>>> Replace arguments for get and set xattr methods, and __vfs_getxattr
>>> and __vfs_setaxtr functions with a reference to the following now
>>> common argument structure:
>>>
>>> struct xattr_gs_args {
>>>        struct dentry *dentry;
>>>        struct inode *inode;
>>>        const char *name;
>>>        union {
>>>                void *buffer;
>>>                const void *value;
>>>        };
>>>        size_t size;
>>>        int flags;
>>> };
>>> Mark,
>>>
>>> I do not see the first patch on fsdevel
>>> and I am confused from all the suggested APIs
>>> I recall Christoph's comment on v8 for not using xattr_gs_args
>>> and just adding flags to existing get() method.
>>> I agree to that comment.
>> As already responded, third (?) patch version was like that,
> The problem is that because of the waaay too long CC list, most revisions
> of the patch and discussion were bounced from fsdevel, most emails
> I did not get and cannot find in archives, so the discussion around
> them is not productive.
>
> Please resend patch to fsdevel discarding the auto added CC list
> of all fs maintainers.

git send-email is not my friend :-(

>> gregkh@
>> said it passed the limit for number of arguments, is looking a bit silly
> Well, you just matched get() to set() args list, so this is not a strong
> argument IMO.
>
>> (my paraphrase), and that it should be passed as a structure. Two others
>> agreed. We gained because both set and get use the same structure after
>> this change (this allows a simplified read-modify-write cycle).
> That sounds like a nice benefit if this was user API, but are there any
> kernel users that intend to make use of that read-modify-write cycle?
> I don't think so.
(one user)
>
>> We will need a quorum on this, 3 (structure) to 2 (flag) now (but really
>> basically between Greg and Christoph?). Coding style issue: Add a flag,
>> or switch to a common xattr argument  structure?
>>
> IIRC, Christoph was asking why the silly struct and not simply add flags
> (as did I). He probably did not see Greg's comments due to the list bounce
> issue. If I read your second hand description of Greg's reaction correctly,
> it doesn't sound so strong opinionated as well.
> Me, I can live with flags or struct - I don't care, but...
>
> Be prepared that if you are going ahead with struct you are going to
> suffer from bike shedding, which has already started and you will be
> instructed (just now) to also fix all the relevant and missing Documentation.
> If, on the other hand, you can get Greg and the rest to concede to adding
> flags arg and match get() arg list to set() arg list, you will have a much
> easier job and the patch line count, especially in fs code will be *much*
> smaller - just saying.

Respining back to the v4 version of the series incorporating some of the 
fixes on the way.

Automated testing in kernel not yet handled and will be noted in the 
respin.

> Thanks,
> Amir.

Mark


^ permalink raw reply

* Investment opportunity
From: Peter Wong @ 2019-11-05  3:17 UTC (permalink / raw)
  To: linux-security-module

Greetings,

Find the attached mail very confidential. reply for more details

Thanks.
Peter Wong




----------------------------------------------------
This email was sent by the shareware version of Postman Professional.


^ permalink raw reply

* Re: kernel panic while using get_random_bytes
From: Ondrej Mosnacek @ 2019-11-05  9:12 UTC (permalink / raw)
  To: Temp Sha; +Cc: Linux Security Module list, linux-crypto
In-Reply-To: <CANe=CUmBHF=L8EUVvupGSYKY_m2PH_4aH=pL7Fky57BY4JQdvA@mail.gmail.com>

Hi Temp,

On Mon, Oct 14, 2019 at 10:21 AM Temp Sha <temp.sha@gmail.com> wrote:
>
> hi,
>
> i use get_random_bytes() function for my randomness requirement in
> kernel version 4.14.142
> however is gives panic as soon as I call   get_random_bytes() in my module.
>
>
> Oct 10 07:20:18 BUG: unable to handle kernel paging request at 00007f5563ced000
> IP: chacha20_block+0x24d/0x280
> PGD 800000010f7f8067 P4D 800000010f7f8067 PUD 161316067 PMD 1015a8067 PTE 0
> Oops: 0002 [#1] PREEMPT SMP PTI
> Modules linked in: ipi_hsl(PO) mymod(PO) e1000 ipv6 ftdi_sio usbserial
> xt_tcpudp xt_mark iptable_nat nf_nat_ipv4 nf_conntrack_ipv4
> nf_defrag_ipv4 nf_nat xt_connlimit nf_conntrack iptable_filter
> ip_tables x_tables
> CPU: 0 PID: 1841 Comm: hexdump Tainted: P           O    4.14.142-ws-symbol #1
> Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> task: ffff8881611da000 task.stack: ffffc900504c4000
> RIP: 0010:chacha20_block+0x24d/0x280
> RSP: 0018:ffffc900504c7c70 EFLAGS: 00010886
> RAX: 0000000000000000 RBX: 00000000a88c95b0 RCX: 00007f5563ced000
> RDX: ffff88810f79da00 RSI: 0000000015c4332e RDI: 000000007613f298
> RBP: ffffc900504c7d00 R08: 000000009d39d68d R09: 00000000bfbdb51f
> R10: 00000000ed798a26 R11: 0000000083c184dc R12: 0000000036fc61e0
> R13: 00000000f9004639 R14: 0000000042c0d351 R15: 000000008a6cef0f
> FS:  00007f5563cef700(0000) GS:ffff888167e00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f5563ced000 CR3: 000000016117c000 CR4: 00000000000006b0
> Call Trace:
>  _extract_crng+0x6d/0xc0
>  extract_crng+0x3a/0x40
>  _get_random_bytes+0x56/0x1c0
>  ? vprintk_func+0x3f/0xd0
>  ? printk+0x3e/0x46
>  get_random_bytes+0x2f/0x40
>  xyz_packets+0x1110/0x11e0 [mymod]
>  proc_reg_read+0x3d/0x60
>  __vfs_read+0x23/0x120
>  ? vm_mmap_pgoff+0x9d/0xd0
>  vfs_read+0x8e/0x110
>  SyS_read+0x48/0xc0
>  do_syscall_64+0x5c/0x260
>  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
> RIP: 0033:0x7f55631f
>
>
> what could be the problem?

Since no one else is replying, I'll try to give you some hints. First,
you are posting on the wrong list - linux-security-module@ is for
Linux Security Modules (see Documentation/security/lsm.rst [1]).
Random number generation is more relevant for linux-crypto@ (now in
Cc). Second, the most likely explanation for why you are getting the
crash is that you pass invalid buffer/length to the function. Please
double-check the code in your module whether the pointer and length is
valid. If you fail to find any issue with your code, then ideally post
the full source of you module if possible so people can try to
reproduce and investigate the issue. You could also try to reproduce
the issue with a recent mainline kernel to see if it is an issue that
has been already fixed. If it works with the new kernel then the
stable 4.14 branch you are using likely lacks a backport of the fix.

Hope that helps,

[1] https://www.kernel.org/doc/html/latest/security/lsm.html

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.


^ permalink raw reply

* Re: [PATCH v23 12/24] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2019-11-05 11:11 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-kernel, x86, linux-sgx, akpm, dave.hansen, nhorman,
	npmccallum, serge.ayoun, shay.katz-zamir, haitao.huang,
	andriy.shevchenko, tglx, kai.svahn, bp, josh, luto, kai.huang,
	rientjes, cedric.xing, puiterwijk, linux-security-module,
	Suresh Siddha
In-Reply-To: <20191031211252.GC10507@linux.intel.com>

On Thu, Oct 31, 2019 at 11:12:52PM +0200, Jarkko Sakkinen wrote:
> On Wed, Oct 30, 2019 at 02:30:45AM -0700, Sean Christopherson wrote:
> > Why?  The number of pages processed is effectively returned via the params
> > on any error, e.g. wouldn't it be more appropriate to return -ERESTARTSYS?
> > And I don't see any reason to add an arbitrary cap on the number of pages,
> > e.g. SGX plays nice with the scheduler and signals, and restricting the
> > number of EPC pages available to a process via cgroups (returning -ENOMEM)
> > is a better solution for managing EPC.
> 
> Returning -ENOMEM does not tell you from which page to retry.

API should be robust enough to be able to cap the amount of data
processed with or without cgroups like send(), recv(), read() and
write() are and the call pattern for it must be a loop not a single shot
call for any megalomaniac length.

I'll add @count to address this. This output field will contain the
number of bytes actually written instead of overwriting input
parameters, which is a bad practice in anyway.

We don't need to actually cap to anything but API must be able to
support such scenario. Caller must be prepared to deal with the
situation where the return value is zero but @count < @length.

/Jarkko

^ permalink raw reply

* Details on the UAPI of implementing notifications on pipes
From: David Howells @ 2019-11-05 16:02 UTC (permalink / raw)
  To: torvalds, viro
  Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
	nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
	linux-block, linux-security-module, linux-fsdevel, linux-api,
	linux-kernel
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>

So to implement notifications on top of pipes, I've hacked it together a bit
in the following ways:

 (1) I'm passing O_TMPFILE to the pipe2() system call to indicate that you
     want a notifications pipe.  This prohibits splice and co. from being
     called on it as I don't want to have to try to fix iov_iter_revert() to
     handle kernel notifications being intermixed with splices.

     The choice of O_TMPFILE was just for convenience, but it needs to be
     something different.  I could, for instance, add a constant,
     O_NOTIFICATION_PIPE with the same *value* as O_TMPFILE.  I don't think
     it's likely that it will make sense to use O_TMPFILE with a pipe, but I
     also don't want to eat up another O_* constant just for this.

     Unfortunately, pipe2() doesn't have any other arguments into from which I
     can steal a bit.

 (2) I've added a pair of ioctls to configure the notifications bits.  They're
     ioctls as I just reused the ioctl code from my devmisc driver.  Should I
     use fcntl() instead, such as is done for F_SETPIPE_SZ?

     The ioctls do two things: set the ring size to a number of slots (so
     similarish to F_SETPIPE_SZ) and set filters.

Any thoughts on how better to represent these bits?

Thanks,
David


^ permalink raw reply

* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Mike Salvatore @ 2019-11-05 16:43 UTC (permalink / raw)
  To: Brendan Higgins, Iurii Zaikin
  Cc: Kees Cook, Luis Chamberlain, Alan Maguire, Matthias Maennich,
	shuah, John Johansen, jmorris, serge, David Gow,
	Theodore Ts'o, Linux Kernel Mailing List,
	linux-security-module, KUnit Development,
	open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <CAFd5g446cyijzgap9r8nm_202zkUsfdZXrn5E1_Mfe-R+eFb_g@mail.gmail.com>

>> but such approach is not mainstream.
>> I personally like the idea of testing the lowest level bits in isolation even if
>> they are not a part of any interface. I think that specifying the
>> interface using
>> unit tests and ensuring implementation correctness are complementary but
>> I haven't had much luck arguing this with our esteemed colleagues.

In general, testing public interfaces is preferable, however, I think it's
important to avoid becoming dogmatic. IMHO, it's more important to have tests
that are clear in what they test than to not write tests (or write confusing
tests) in order to adhere to a generalized principle.

> So I think this is a very subtle point which is very widely
> misunderstood. Most people write code and then write their tests,
> following this practice along with only testing public interfaces
> often causes people to just not test all of their code, which is
> wrong.

The very nature of this situation is that the code was written before the tests.

> The idea of only testing public interfaces is supposed to make people
> think more carefully about what the composite layers of the program
> is. If you are having difficulty getting decent coverage by only
> testing your public interfaces, then it likely tells you that you have
> one of two problems:
> 
> 1) You have code that you don't need, and you should remove it.
> 
> 2) One of the layers in your program is too think, and you should
> introduce a new layer with a new public interface that you can test
> through.
> 
> I think the second point here is problematic with how C is written in
> the kernel. We don't really have any concept of public vs. private
> inside the kernel outside of static vs. not static, which is much more
> restricted.

I don't think we can expect developers to refactor large portions of complex
kernel code in order to improve its testability. I imagine this will happen
naturally over time, but I think we need to allow for developers to test
"private" code in the meanwhile.

My opinion is that it's more important to have tests than not. As evidence, I
submit the following commit:
https://github.com/torvalds/linux/commit/156e42996bd84eccb6acf319f19ce0cb140d00e3.

While not a major bug, this bug was discovered as a direct result of writing
these unit tests. So, in summary, I see value in "testing the lowest level bits
in isolation", even if it doesn't necessarily represent the Gold Standard in
Unit Testing.


^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 17:18 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
	Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev
In-Reply-To: <20191104172146.30797-5-mic@digikod.net>

On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> Add a first Landlock hook that can be used to enforce a security policy
> or to audit some process activities.  For a sandboxing use-case, it is
> needed to inform the kernel if a task can legitimately debug another.
> ptrace(2) can also be used by an attacker to impersonate another task
> and remain undetected while performing malicious activities.
> 
> Using ptrace(2) and related features on a target process can lead to a
> privilege escalation.  A sandboxed task must then be able to tell the
> kernel if another task is more privileged, via ptrace_may_access().
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
...
> +static int check_ptrace(struct landlock_domain *domain,
> +		struct task_struct *tracer, struct task_struct *tracee)
> +{
> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
> +		.prog_ctx = {
> +			.tracer = (uintptr_t)tracer,
> +			.tracee = (uintptr_t)tracee,
> +		},
> +	};

So you're passing two kernel pointers obfuscated as u64 into bpf program
yet claiming that the end goal is to make landlock unprivileged?!
The most basic security hole in the tool that is aiming to provide security.

I think the only way bpf-based LSM can land is both landlock and KRSI
developers work together on a design that solves all use cases. BPF is capable
to be a superset of all existing LSMs whereas landlock and KRSI propsals today
are custom solutions to specific security concerns. BPF subsystem was extended
with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
program types with a lot of overlapping functionality. We couldn't figure out
how to generalize them into single 'networking' program. Now we can and we
should. Accepting two partially overlapping bpf-based LSMs would be repeating
the same mistake again.


^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Casey Schaufler @ 2019-11-05 17:55 UTC (permalink / raw)
  To: Alexei Starovoitov, Mickaël Salaün
  Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
	Daniel Borkmann, David Drysdale, Florent Revest, James Morris,
	Jann Horn, John Johansen, Jonathan Corbet, Kees Cook, KP Singh,
	Michael Kerrisk, Mickaël Salaün, Paul Moore,
	Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
	Tejun Heo, Tetsuo Handa, Tycho Andersen, Will Drewry, bpf,
	kernel-hardening, linux-api, linux-security-module, netdev, casey
In-Reply-To: <20191105171824.dfve44gjiftpnvy7@ast-mbp.dhcp.thefacebook.com>

On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>> Add a first Landlock hook that can be used to enforce a security policy
>> or to audit some process activities.  For a sandboxing use-case, it is
>> needed to inform the kernel if a task can legitimately debug another.
>> ptrace(2) can also be used by an attacker to impersonate another task
>> and remain undetected while performing malicious activities.
>>
>> Using ptrace(2) and related features on a target process can lead to a
>> privilege escalation.  A sandboxed task must then be able to tell the
>> kernel if another task is more privileged, via ptrace_may_access().
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ...
>> +static int check_ptrace(struct landlock_domain *domain,
>> +		struct task_struct *tracer, struct task_struct *tracee)
>> +{
>> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
>> +		.prog_ctx = {
>> +			.tracer = (uintptr_t)tracer,
>> +			.tracee = (uintptr_t)tracee,
>> +		},
>> +	};
> So you're passing two kernel pointers obfuscated as u64 into bpf program
> yet claiming that the end goal is to make landlock unprivileged?!
> The most basic security hole in the tool that is aiming to provide security.
>
> I think the only way bpf-based LSM can land is both landlock and KRSI
> developers work together on a design that solves all use cases. BPF is capable
> to be a superset of all existing LSMs

I can't agree with this. Nope. There are many security models
for which BPF introduces excessive complexity. You don't need
or want the generality of a general purpose programming language
to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
that matter. SELinux? I can't imagine anyone trying to do that
in eBPF, although I'm willing to be surprised. Being able to
enforce a policy isn't the only criteria for an LSM. It's got
to perform well and integrate with the rest of the system. I
see many issues with a BPF <-> vfs interface.

> whereas landlock and KRSI propsals today
> are custom solutions to specific security concerns.

Yes. As they should be. No one has every solved the entire
security problem, and no one ever will. The only hope we have
to address security issues is to have the flexibility to add
the mechanisms needed for the concerns of the day. Ideally,
we should be able to drop mechanisms when we decide that they
no longer add value.

> BPF subsystem was extended
> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> program types with a lot of overlapping functionality. We couldn't figure out
> how to generalize them into single 'networking' program. Now we can and we
> should. Accepting two partially overlapping bpf-based LSMs would be repeating
> the same mistake again.

I don't get your analogy at all. You have a variety of programs because
you have a variety of protocols and administrative interfaces. Of course
you don't have a single 'networking" program. Security has a variety of
issues and policies. A single 'security' program makes no sense whatever.



^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-05 18:01 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
	Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev
In-Reply-To: <20191105171824.dfve44gjiftpnvy7@ast-mbp.dhcp.thefacebook.com>


On 05/11/2019 18:18, Alexei Starovoitov wrote:
> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>> Add a first Landlock hook that can be used to enforce a security policy
>> or to audit some process activities.  For a sandboxing use-case, it is
>> needed to inform the kernel if a task can legitimately debug another.
>> ptrace(2) can also be used by an attacker to impersonate another task
>> and remain undetected while performing malicious activities.
>>
>> Using ptrace(2) and related features on a target process can lead to a
>> privilege escalation.  A sandboxed task must then be able to tell the
>> kernel if another task is more privileged, via ptrace_may_access().
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ...
>> +static int check_ptrace(struct landlock_domain *domain,
>> +		struct task_struct *tracer, struct task_struct *tracee)
>> +{
>> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
>> +		.prog_ctx = {
>> +			.tracer = (uintptr_t)tracer,
>> +			.tracee = (uintptr_t)tracee,
>> +		},
>> +	};
> 
> So you're passing two kernel pointers obfuscated as u64 into bpf program
> yet claiming that the end goal is to make landlock unprivileged?!
> The most basic security hole in the tool that is aiming to provide security.

How could you used these pointers without dedicated BPF helpers? This
context items are typed as PTR_TO_TASK and can't be used without a
dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
arithmetic is explicitly forbidden (and I added tests for that). Did I
miss something?

> 
> I think the only way bpf-based LSM can land is both landlock and KRSI
> developers work together on a design that solves all use cases.

As I said in a previous cover letter [1], that would be great. I think
that the current Landlock bases (almost everything from this series
except the seccomp interface) should meet both needs, but I would like
to have the point of view of the KRSI developers.

[1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/

> BPF is capable
> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> are custom solutions to specific security concerns. BPF subsystem was extended
> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> program types with a lot of overlapping functionality. We couldn't figure out
> how to generalize them into single 'networking' program. Now we can and we
> should. Accepting two partially overlapping bpf-based LSMs would be repeating
> the same mistake again.

I'll let the LSM maintainers comment on whether BPF could be a superset
of all LSM, but given the complexity of an access-control system, I have
some doubts though. Anyway, we need to start somewhere and then iterate.
This patch series is a first step.

^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 19:31 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
	Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev
In-Reply-To: <c5c6b433-7e6a-c8f8-f063-e704c3df4cc6@schaufler-ca.com>

On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
> > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >> Add a first Landlock hook that can be used to enforce a security policy
> >> or to audit some process activities.  For a sandboxing use-case, it is
> >> needed to inform the kernel if a task can legitimately debug another.
> >> ptrace(2) can also be used by an attacker to impersonate another task
> >> and remain undetected while performing malicious activities.
> >>
> >> Using ptrace(2) and related features on a target process can lead to a
> >> privilege escalation.  A sandboxed task must then be able to tell the
> >> kernel if another task is more privileged, via ptrace_may_access().
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static int check_ptrace(struct landlock_domain *domain,
> >> +		struct task_struct *tracer, struct task_struct *tracee)
> >> +{
> >> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >> +		.prog_ctx = {
> >> +			.tracer = (uintptr_t)tracer,
> >> +			.tracee = (uintptr_t)tracee,
> >> +		},
> >> +	};
> > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > yet claiming that the end goal is to make landlock unprivileged?!
> > The most basic security hole in the tool that is aiming to provide security.
> >
> > I think the only way bpf-based LSM can land is both landlock and KRSI
> > developers work together on a design that solves all use cases. BPF is capable
> > to be a superset of all existing LSMs
> 
> I can't agree with this. Nope. There are many security models
> for which BPF introduces excessive complexity. You don't need
> or want the generality of a general purpose programming language
> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
> that matter. SELinux? I can't imagine anyone trying to do that
> in eBPF, although I'm willing to be surprised. Being able to
> enforce a policy isn't the only criteria for an LSM. 

what are the other criteria?

> It's got
> to perform well and integrate with the rest of the system. 

what do you mean by that?

> I see many issues with a BPF <-> vfs interface.

There is no such interface today. What do you have in mind?

> the mechanisms needed for the concerns of the day. Ideally,
> we should be able to drop mechanisms when we decide that they
> no longer add value.

Exactly. bpf-based lsm must not add to kernel abi.


^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 19:34 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
	Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev
In-Reply-To: <23acf523-dbc4-855b-ca49-2bbfa5e7117e@digikod.net>

On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
> 
> On 05/11/2019 18:18, Alexei Starovoitov wrote:
> > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >> Add a first Landlock hook that can be used to enforce a security policy
> >> or to audit some process activities.  For a sandboxing use-case, it is
> >> needed to inform the kernel if a task can legitimately debug another.
> >> ptrace(2) can also be used by an attacker to impersonate another task
> >> and remain undetected while performing malicious activities.
> >>
> >> Using ptrace(2) and related features on a target process can lead to a
> >> privilege escalation.  A sandboxed task must then be able to tell the
> >> kernel if another task is more privileged, via ptrace_may_access().
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static int check_ptrace(struct landlock_domain *domain,
> >> +		struct task_struct *tracer, struct task_struct *tracee)
> >> +{
> >> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >> +		.prog_ctx = {
> >> +			.tracer = (uintptr_t)tracer,
> >> +			.tracee = (uintptr_t)tracee,
> >> +		},
> >> +	};
> > 
> > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > yet claiming that the end goal is to make landlock unprivileged?!
> > The most basic security hole in the tool that is aiming to provide security.
> 
> How could you used these pointers without dedicated BPF helpers? This
> context items are typed as PTR_TO_TASK and can't be used without a
> dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
> arithmetic is explicitly forbidden (and I added tests for that). Did I
> miss something?

It's a pointer leak.

> 
> > 
> > I think the only way bpf-based LSM can land is both landlock and KRSI
> > developers work together on a design that solves all use cases.
> 
> As I said in a previous cover letter [1], that would be great. I think
> that the current Landlock bases (almost everything from this series
> except the seccomp interface) should meet both needs, but I would like
> to have the point of view of the KRSI developers.
> 
> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
> 
> > BPF is capable
> > to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> > are custom solutions to specific security concerns. BPF subsystem was extended
> > with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> > program types with a lot of overlapping functionality. We couldn't figure out
> > how to generalize them into single 'networking' program. Now we can and we
> > should. Accepting two partially overlapping bpf-based LSMs would be repeating
> > the same mistake again.
> 
> I'll let the LSM maintainers comment on whether BPF could be a superset
> of all LSM, but given the complexity of an access-control system, I have
> some doubts though. Anyway, we need to start somewhere and then iterate.
> This patch series is a first step.

I would like KRSI folks to speak up. So far I don't see any sharing happening
between landlock and KRSI. You're claiming this set is a first step. They're
claiming the same about their patches. I'd like to set a patchset that was
jointly developed.


^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Casey Schaufler @ 2019-11-05 19:55 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
	Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev, casey
In-Reply-To: <20191105193130.qam2eafnmgvrvjwk@ast-mbp.dhcp.thefacebook.com>

On 11/5/2019 11:31 AM, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
>> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
>>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>>>> Add a first Landlock hook that can be used to enforce a security policy
>>>> or to audit some process activities.  For a sandboxing use-case, it is
>>>> needed to inform the kernel if a task can legitimately debug another.
>>>> ptrace(2) can also be used by an attacker to impersonate another task
>>>> and remain undetected while performing malicious activities.
>>>>
>>>> Using ptrace(2) and related features on a target process can lead to a
>>>> privilege escalation.  A sandboxed task must then be able to tell the
>>>> kernel if another task is more privileged, via ptrace_may_access().
>>>>
>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> ...
>>>> +static int check_ptrace(struct landlock_domain *domain,
>>>> +		struct task_struct *tracer, struct task_struct *tracee)
>>>> +{
>>>> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
>>>> +		.prog_ctx = {
>>>> +			.tracer = (uintptr_t)tracer,
>>>> +			.tracee = (uintptr_t)tracee,
>>>> +		},
>>>> +	};
>>> So you're passing two kernel pointers obfuscated as u64 into bpf program
>>> yet claiming that the end goal is to make landlock unprivileged?!
>>> The most basic security hole in the tool that is aiming to provide security.
>>>
>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>> developers work together on a design that solves all use cases. BPF is capable
>>> to be a superset of all existing LSMs
>> I can't agree with this. Nope. There are many security models
>> for which BPF introduces excessive complexity. You don't need
>> or want the generality of a general purpose programming language
>> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
>> that matter. SELinux? I can't imagine anyone trying to do that
>> in eBPF, although I'm willing to be surprised. Being able to
>> enforce a policy isn't the only criteria for an LSM. 
> what are the other criteria?

They include, but are not limited to, performance impact
and the ability to be analyzed. The interactions with other
subsystems meeting the requirements thereof is always a concern.


>
>> It's got
>> to perform well and integrate with the rest of the system. 
> what do you mean by that?

It has to be fast, or the networking people are
going to have fits. You can't require the addition
of a pointer into the skb because it'll get rejected
out of hand. You can't completely refactor the vfs locking
to accommodate you needs.

>
>> I see many issues with a BPF <-> vfs interface.
> There is no such interface today. What do you have in mind?

You can't implement SELinux or Smack using BPF without a way
to manipulate inode data.

>
>> the mechanisms needed for the concerns of the day. Ideally,
>> we should be able to drop mechanisms when we decide that they
>> no longer add value.
> Exactly. bpf-based lsm must not add to kernel abi.

Huh? I have no idea where that came from.



^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Alexei Starovoitov @ 2019-11-05 21:54 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
	Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev
In-Reply-To: <637736ef-c48e-ac3b-3eef-8a6a095a96f1@schaufler-ca.com>

On Tue, Nov 05, 2019 at 11:55:17AM -0800, Casey Schaufler wrote:
> On 11/5/2019 11:31 AM, Alexei Starovoitov wrote:
> > On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
> >> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
> >>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >>>> Add a first Landlock hook that can be used to enforce a security policy
> >>>> or to audit some process activities.  For a sandboxing use-case, it is
> >>>> needed to inform the kernel if a task can legitimately debug another.
> >>>> ptrace(2) can also be used by an attacker to impersonate another task
> >>>> and remain undetected while performing malicious activities.
> >>>>
> >>>> Using ptrace(2) and related features on a target process can lead to a
> >>>> privilege escalation.  A sandboxed task must then be able to tell the
> >>>> kernel if another task is more privileged, via ptrace_may_access().
> >>>>
> >>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> >>> ...
> >>>> +static int check_ptrace(struct landlock_domain *domain,
> >>>> +		struct task_struct *tracer, struct task_struct *tracee)
> >>>> +{
> >>>> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >>>> +		.prog_ctx = {
> >>>> +			.tracer = (uintptr_t)tracer,
> >>>> +			.tracee = (uintptr_t)tracee,
> >>>> +		},
> >>>> +	};
> >>> So you're passing two kernel pointers obfuscated as u64 into bpf program
> >>> yet claiming that the end goal is to make landlock unprivileged?!
> >>> The most basic security hole in the tool that is aiming to provide security.
> >>>
> >>> I think the only way bpf-based LSM can land is both landlock and KRSI
> >>> developers work together on a design that solves all use cases. BPF is capable
> >>> to be a superset of all existing LSMs
> >> I can't agree with this. Nope. There are many security models
> >> for which BPF introduces excessive complexity. You don't need
> >> or want the generality of a general purpose programming language
> >> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
> >> that matter. SELinux? I can't imagine anyone trying to do that
> >> in eBPF, although I'm willing to be surprised. Being able to
> >> enforce a policy isn't the only criteria for an LSM. 
> > what are the other criteria?
> 
> They include, but are not limited to, performance impact
> and the ability to be analyzed. 

Right and BPF is the only thing that exists in the kernel where the verifier
knows precisely the number of instructions the critical path through the
program will take. Currently we don't quantify this cost for bpf helpers, but
it's easy to add. Can you do this for smack? Can you tell upfront the longest
execution time for all security rules?

> It has to be fast, or the networking people are
> going to have fits. You can't require the addition
> of a pointer into the skb because it'll get rejected
> out of hand. You can't completely refactor the vfs locking
> to accommodate you needs.

I'm not sure why you got such impression. I'm not proposing to refactor vfs or
add fields to skb. Once we have equivalent to smack policy implemented in
bpf-based lsm let's do performance benchmarking and compare actual numbers
instead of hypothesizing about them. Which policy do you think would be
the most representative of smack use case?

> 
> >
> >> I see many issues with a BPF <-> vfs interface.
> > There is no such interface today. What do you have in mind?
> 
> You can't implement SELinux or Smack using BPF without a way
> to manipulate inode data.

Are you talking about inode->i_security ? That's not manipulating inode data.
It's attaching extra metadata to inode object without changing inode itself.
BPF can do it already via hash maps. It's not as fast as direct pointer access,
but for many use cases it's good enough. If it turns out to be a performance
limiting factor we will accelerate it.

> >> the mechanisms needed for the concerns of the day. Ideally,
> >> we should be able to drop mechanisms when we decide that they
> >> no longer add value.
> > Exactly. bpf-based lsm must not add to kernel abi.
> 
> Huh? I have no idea where that came from.

It sounds to me that some folks in the community got wrong impression that
anything that BPF accesses is magically turning that thing into stable kernel
ABI. That is not true. BPF progs had access _all_ kernel data pointers and
structures for years without turning the whole kernel into stable ABI. I want
to make sure that this part is understood. This is also a requirement for
bpf-based LSM. It must not make LSM hooks into stable ABI.


^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Mickaël Salaün @ 2019-11-05 22:18 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: linux-kernel, Alexei Starovoitov, Andy Lutomirski,
	Casey Schaufler, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev
In-Reply-To: <20191105193446.s4pswwwhrmgk6hcx@ast-mbp.dhcp.thefacebook.com>


On 05/11/2019 20:34, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
>>
>> On 05/11/2019 18:18, Alexei Starovoitov wrote:
>>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>>>> Add a first Landlock hook that can be used to enforce a security policy
>>>> or to audit some process activities.  For a sandboxing use-case, it is
>>>> needed to inform the kernel if a task can legitimately debug another.
>>>> ptrace(2) can also be used by an attacker to impersonate another task
>>>> and remain undetected while performing malicious activities.
>>>>
>>>> Using ptrace(2) and related features on a target process can lead to a
>>>> privilege escalation.  A sandboxed task must then be able to tell the
>>>> kernel if another task is more privileged, via ptrace_may_access().
>>>>
>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> ...
>>>> +static int check_ptrace(struct landlock_domain *domain,
>>>> +		struct task_struct *tracer, struct task_struct *tracee)
>>>> +{
>>>> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
>>>> +		.prog_ctx = {
>>>> +			.tracer = (uintptr_t)tracer,
>>>> +			.tracee = (uintptr_t)tracee,
>>>> +		},
>>>> +	};
>>>
>>> So you're passing two kernel pointers obfuscated as u64 into bpf program
>>> yet claiming that the end goal is to make landlock unprivileged?!
>>> The most basic security hole in the tool that is aiming to provide security.
>>
>> How could you used these pointers without dedicated BPF helpers? This
>> context items are typed as PTR_TO_TASK and can't be used without a
>> dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
>> arithmetic is explicitly forbidden (and I added tests for that). Did I
>> miss something?
> 
> It's a pointer leak.

The lifetimes of the pointers are scoped by the two LSM hooks that
expose them. The LSM framework guarantee that they are safe to use in
this context.

> 
>>
>>>
>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>> developers work together on a design that solves all use cases.
>>
>> As I said in a previous cover letter [1], that would be great. I think
>> that the current Landlock bases (almost everything from this series
>> except the seccomp interface) should meet both needs, but I would like
>> to have the point of view of the KRSI developers.
>>
>> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
>>
>>> BPF is capable
>>> to be a superset of all existing LSMs whereas landlock and KRSI propsals today
>>> are custom solutions to specific security concerns. BPF subsystem was extended
>>> with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
>>> program types with a lot of overlapping functionality. We couldn't figure out
>>> how to generalize them into single 'networking' program. Now we can and we
>>> should. Accepting two partially overlapping bpf-based LSMs would be repeating
>>> the same mistake again.
>>
>> I'll let the LSM maintainers comment on whether BPF could be a superset
>> of all LSM, but given the complexity of an access-control system, I have
>> some doubts though. Anyway, we need to start somewhere and then iterate.
>> This patch series is a first step.
> 
> I would like KRSI folks to speak up. So far I don't see any sharing happening
> between landlock and KRSI. You're claiming this set is a first step. They're
> claiming the same about their patches. I'd like to set a patchset that was
> jointly developed.

With all due respect, Landlock got much more feedback than KRSI and I
think this thirteenth Landlock patch series is more mature than the
first KRSI RFC. I'm open to concrete suggestions and I'm willing to
collaborate with the KRSI folks if they want to. However, I'm OK if they
don't want to use Landlock as a common ground, and I don't think it
should be a blocker for any of the projects.

Perfect is the enemy of good. ;)

^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: Casey Schaufler @ 2019-11-05 22:32 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
	Andy Lutomirski, Daniel Borkmann, David Drysdale, Florent Revest,
	James Morris, Jann Horn, John Johansen, Jonathan Corbet,
	Kees Cook, KP Singh, Michael Kerrisk, Mickaël Salaün,
	Paul Moore, Sargun Dhillon, Serge E . Hallyn, Shuah Khan,
	Stephen Smalley, Tejun Heo, Tetsuo Handa, Tycho Andersen,
	Will Drewry, bpf, kernel-hardening, linux-api,
	linux-security-module, netdev, casey
In-Reply-To: <20191105215453.szhdkrvuekwfz6le@ast-mbp.dhcp.thefacebook.com>

On 11/5/2019 1:54 PM, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 11:55:17AM -0800, Casey Schaufler wrote:
>> On 11/5/2019 11:31 AM, Alexei Starovoitov wrote:
>>> On Tue, Nov 05, 2019 at 09:55:42AM -0800, Casey Schaufler wrote:
>>>> On 11/5/2019 9:18 AM, Alexei Starovoitov wrote:
>>>>> On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
>>>>>> Add a first Landlock hook that can be used to enforce a security policy
>>>>>> or to audit some process activities.  For a sandboxing use-case, it is
>>>>>> needed to inform the kernel if a task can legitimately debug another.
>>>>>> ptrace(2) can also be used by an attacker to impersonate another task
>>>>>> and remain undetected while performing malicious activities.
>>>>>>
>>>>>> Using ptrace(2) and related features on a target process can lead to a
>>>>>> privilege escalation.  A sandboxed task must then be able to tell the
>>>>>> kernel if another task is more privileged, via ptrace_may_access().
>>>>>>
>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>> ...
>>>>>> +static int check_ptrace(struct landlock_domain *domain,
>>>>>> +		struct task_struct *tracer, struct task_struct *tracee)
>>>>>> +{
>>>>>> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
>>>>>> +		.prog_ctx = {
>>>>>> +			.tracer = (uintptr_t)tracer,
>>>>>> +			.tracee = (uintptr_t)tracee,
>>>>>> +		},
>>>>>> +	};
>>>>> So you're passing two kernel pointers obfuscated as u64 into bpf program
>>>>> yet claiming that the end goal is to make landlock unprivileged?!
>>>>> The most basic security hole in the tool that is aiming to provide security.
>>>>>
>>>>> I think the only way bpf-based LSM can land is both landlock and KRSI
>>>>> developers work together on a design that solves all use cases. BPF is capable
>>>>> to be a superset of all existing LSMs
>>>> I can't agree with this. Nope. There are many security models
>>>> for which BPF introduces excessive complexity. You don't need
>>>> or want the generality of a general purpose programming language
>>>> to implement Smack or TOMOYO. Or a simple Bell & LaPadula for
>>>> that matter. SELinux? I can't imagine anyone trying to do that
>>>> in eBPF, although I'm willing to be surprised. Being able to
>>>> enforce a policy isn't the only criteria for an LSM. 
>>> what are the other criteria?
>> They include, but are not limited to, performance impact
>> and the ability to be analyzed. 
> Right and BPF is the only thing that exists in the kernel where the verifier
> knows precisely the number of instructions the critical path through the
> program will take. Currently we don't quantify this cost for bpf helpers, but
> it's easy to add. Can you do this for smack? Can you tell upfront the longest
> execution time for all security rules?

There's much more to analyze than number of instructions.
There's also completion of policy enforcement. There are
lots of tools for measuring performance within the kernel.

>> It has to be fast, or the networking people are
>> going to have fits. You can't require the addition
>> of a pointer into the skb because it'll get rejected
>> out of hand. You can't completely refactor the vfs locking
>> to accommodate you needs.
> I'm not sure why you got such impression. I'm not proposing to refactor vfs or
> add fields to skb.

I'm not saying you did. Those are examples of things you would
have trouble with.

>  Once we have equivalent to smack policy implemented in
> bpf-based lsm let's do performance benchmarking and compare actual numbers
> instead of hypothesizing about them. Which policy do you think would be
> the most representative of smack use case?

The Tizen3 Three domain model will do just fine.
https://wiki.tizen.org/Security:SmackThreeDomainModel


>
>>>> I see many issues with a BPF <-> vfs interface.
>>> There is no such interface today. What do you have in mind?
>> You can't implement SELinux or Smack using BPF without a way
>> to manipulate inode data.
> Are you talking about inode->i_security ? That's not manipulating inode data.

Poppycock.

> It's attaching extra metadata to inode object without changing inode itself.

Where I come from, we call that inode object data.

> BPF can do it already via hash maps. It's not as fast as direct pointer access,

Then you're not listening. Performance MATTERS!

> but for many use cases it's good enough. If it turns out to be a performance
> limiting factor we will accelerate it.

How many times have I heard that bit of rubbish?
No. You can't start with a bad design and tweak it to acceptability later.


>>>> the mechanisms needed for the concerns of the day. Ideally,
>>>> we should be able to drop mechanisms when we decide that they
>>>> no longer add value.
>>> Exactly. bpf-based lsm must not add to kernel abi.
>> Huh? I have no idea where that came from.
> It sounds to me that some folks in the community got wrong impression that
> anything that BPF accesses is magically turning that thing into stable kernel
> ABI. That is not true. BPF progs had access _all_ kernel data pointers and
> structures for years without turning the whole kernel into stable ABI. I want
> to make sure that this part is understood. This is also a requirement for
> bpf-based LSM. It must not make LSM hooks into stable ABI.
>


^ permalink raw reply

* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-11-05 23:44 UTC (permalink / raw)
  To: Alan Maguire
  Cc: Kees Cook, Luis Chamberlain, Matthias Maennich, shuah,
	John Johansen, jmorris, serge, Iurii Zaikin, David Gow,
	Theodore Ts'o, Linux Kernel Mailing List,
	linux-security-module, KUnit Development,
	open list:KERNEL SELFTEST FRAMEWORK, Mike Salvatore
In-Reply-To: <alpine.LRH.2.20.1911011142160.15982@dhcp-10-175-177-231.vpn.oracle.com>

On Fri, Nov 1, 2019 at 5:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On Thu, 31 Oct 2019, Brendan Higgins wrote:
>
> > On Wed, Oct 30, 2019 at 12:09 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
> > > > On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
> > > > > With that, I think the best solution in this case will be the
> > > > > "__visible_for_testing" route. It has no overhead when testing is
> > > > > turned off (in fact it is no different in anyway when testing is
> > > > > turned off). The downsides I see are:
> > > > >
> > > > > 1) You may not be able to test non-module code not compiled for
> > > > > testing later with the test modules that Alan is working on (But the
> > > > > only way I think that will work is by preventing the symbol from being
> > > > > inlined, right?).
> > > > >
> > > > > 2) I think "__visible_for_testing" will be prone to abuse. Here, I
> > > > > think there are reasons why we might want to expose these symbols for
> > > > > testing, but not otherwise. Nevertheless, I think most symbols that
> > > > > should be tested should probably be made visible by default. Since you
> > > > > usually only want to test your public interfaces. I could very well
> > > > > see this getting used as a kludge that gets used far too frequently.
> > > >
> > > > There are two parts to your statement on 2):
> > > >
> > > >   a) possible abuse of say __visible_for_testing
> > >
> > > I really don't like the idea of littering the kernel with these. It'll
> >
> > Yeah, I kind of hope that it would make people think more
> > intentionally about what is a public interface so that they wouldn't
> > litter the kernel with those. But I agree that in the world where
> > people *didn't* do that. Lots of these sprinkled around would be
> > annoying.
> >
> > > also require chunks in header files wrapped in #ifdefs. This is really
> >
> > Why would it require header files wrapped in #ifdefs?
> >
> > We could put all the ifdeffery logic in the __visible_for_testing
> > macro so that nothing in the original code has to change except for
> > adding an #include and replacing a couple of `static`s with
> > `__visible_for_testing`.
> >
>
> FWIW I think this approach, if used sparingly, is fine.  However I'd
> propose a hierarchy of options when looking to expose interfaces for
> testing.
>
> 1. For small, largely self-contained functions, move their definitions
> from .c files to a .h file where those functions are defined as "static
> inline".  That way the original code and tests can included them and we
> have solved function availability for both the cases where the tests are
> built-in and compiled as a module.  The apparmor interfaces here seem to
> be candidates for that approach.
>
> 2. For more complex cases, __visible_for_testing (for built-in visbility)
> and some sort of equivalent EXPORT_FOR_TESTING (for module
> visibility) would work, or the kunit_find_symbol() based lookup approach I
> suggested in the module patches.  Either of these allows for building
> tests as modules or builtin.
>
> 3. For some cases, module support will probably be impossible or difficult
> to maintain.  In such cases, builtin tests make most sense so any
> questions about symbol visibility would largely concern changing static
> definitions to be __visibile_for_testing, with no need for any symbol
> export for module visibility.

Very well said, I think this sums up a lot of good points. Basically,
I think you illustrate that it's not just one of the ways that were
proposed is the most appropriate for all cases, but really several are
valid strategies in different instances.

> > > ugly.
> > >
> > > >   b) you typically only want to test your public interfaces
> > >
> > > True, but being able to test the little helper functions is a nice
> > > starting point and a good building block.
> >
> > Yeah, I think I have come to accept that. We can argue about how this
> > should change and how people need to learn to be more intentional
> > about which interfaces are public and many other high minded ideas,
> > but when it comes down to it, we need to provide a starting point that
> > is easy.
> >
> > If our nice starting point becomes a problem, we can always improve it later.
> >
> > > Why can't unit tests live with the code they're testing? They're already
> > > logically tied together; what's the harm there? This needn't be the case
> > > for ALL tests, etc. The test driver could still live externally. The
> > > test in the other .c would just have exported functions... ?
> >
> > Well, for one, it totally tanks certain cases for building KUnit tests
> > as modules. I don't care about this point *too* much personally, but I
> > accept that there are others that want this, and I don't want to make
> > these people's lives too difficult.
> >
>
> Appreciated.  I think at this point it might be useful to lay out my
> thinking on why being able to build tests as modules may be helpful moving
> forward.
>
> - First and foremost, if the functionality itself is predominantly
> delivered in module form, or indeed is simply tristate, having a way to
> test kernel code when built as a module seems to me to be necessary. To
> test module code with built-in test code seems broken, and even if it
> could be made to work we'd end up having to invent a bunch of the mechanisms
> we'd need for building tests as modules anyway.

I think that is a fair point. I think I was thinking of it as an all
or nothing type thing. I know that we had moved past it in words, but
I think I was still hung up on the idea that we were going to try to
aggressively make tests buildable as modules. Here, and combined with
what Mike said (in a later email), I think I realized that a better
metric is what the code under test does.

It's probably not a big deal to make *everything* available as a
module. The right thing is probably that, if the code is available as
a module, the test should probably be available as a module. If the
code is not available as a module, it is not necessary to provide the
test as a module.

But that and what Mike said, I think, gets at something deeper. Each
subsystem has its own way of doing things, and that is a reality we
have to deal with. As long as there is some way to "run all the tests"
what conventions we enforce at the outset may not really be all that
important. Sure, some amount of consistency is important, but what is
more important is that we make something that is easy to use. We can
always go back and clean things up later.

After writing this, it sounds kind of obvious and like things that
people have said already; nevertheless, I think it is still worthwhile
to say, if nothing else to show that I think we are all on the same
page.

So yeah, I think that optionally including tests in the code under
test is fine (if that is what works best for the developer), I think
that __visible_for_testing is fine if that's what works best, and
testing through public interfaces is also fine. We might want to
gently push people in one direction or another over time, but all seem
like things that are reasonable to support now. In this case, since
people seem to be more in favor of including tests in source, that's
probably the right thing to do here.

I will make the __visible_for_testing thing available in a separate
patch at some point. Someone can pick it up if they want to use it.

> - Running tests on demand.  From previous discussions, I think this is
> wanted for kselftest, and if we have a set of modules with a conventional
> prefix (e.g. kunit-*), running tests becomes simply a "find + modprobe" in
> the kernel module tree.  Results could be harvested from debugfs (I have a
> WIP patch to store logging data in the per-test data structures such that
> "cat /sys/kernel/debug/kunit-results/kunit-foo" will display results for
> that test suite).  There are other ways to achieve this goal, and it's
> a crude method without any test selection beyond which modules are
> loaded, but this path is noticeably shorter to having a simple way to
> execute tests in a kselftest-friendly way I think.

Yep, I think we are all in agreement here. Shuah, Knut, and myself all
agreed at LPC that running tests on demand via kselftest was a
worthwhile goal. I am not strongly opposed to the common prefix idea.
I think that is something we might want to run past Linus though, as
he has not been a fan of certain file prefixes that he considers to
convey redundant information.

> - Developing tests. I've also found this model to be neat for test
> development; add a test, build, load the module to test the test, add
> another test, build, unload/load etc.

Not really sure what you mean here. I suspect that I probably won't
agree, as I have found that rebuilding the kernel for every test is
not overly burdensome. Nevertheless, I also don't see any reason to
oppose you here. If some developer likes that model (even if I don't),
it is best to support it if possible, as we should ideally make things
easier for every development flow.

> - The late_initcall() initialization of tests may not always be appropriate
> for subsystems under test, and as the number of tests grow (a good
> problem to have!), it will likely become infeasible.

Agreed. I just went with it for now because it was easy and
uncontroversial. I already have some WIP patches to get rid of it (I
am not sure how that will affect what you are doing, but I suspect
your module patches will be done first - since they already look close
- so I will probably try to figure that out after your module patches
get merged).

> Anyway I'm not sure if any of the above resonates with others as being
> useful, but hopefully it clarifies why module support might matter moving
> forward.

I definitely agree with the point about supporting building tests as
modules if the code under test builds as modules, especially if the
developers want it. So yes, it does resonate with me! :-)

> If it makes sense, I can look at tweaking the module patchset to remove
> the kunit_find_symbol() stuff so that we can punt on specific mechanisms
> for now; my main aim at this point is to ensure we're thinking about
> providing mechanisms for testing modules.

Yeah, I started looking at the latest version of your patches (sorry
for the late follow up), and yeah, I think it probably makes sense to
split that out into a separate patch/patchset.

Thanks for the comments! They really helped clear some things up for me!

^ permalink raw reply

* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-11-05 23:59 UTC (permalink / raw)
  To: Mike Salvatore
  Cc: Iurii Zaikin, Kees Cook, Luis Chamberlain, Alan Maguire,
	Matthias Maennich, shuah, John Johansen, jmorris, serge,
	David Gow, Theodore Ts'o, Linux Kernel Mailing List,
	linux-security-module, KUnit Development,
	open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <205525ba-dc2f-34a9-b7dc-4421285535d7@canonical.com>

On Tue, Nov 5, 2019 at 8:43 AM Mike Salvatore
<mike.salvatore@canonical.com> wrote:
>
> >> but such approach is not mainstream.
> >> I personally like the idea of testing the lowest level bits in isolation even if
> >> they are not a part of any interface. I think that specifying the
> >> interface using
> >> unit tests and ensuring implementation correctness are complementary but
> >> I haven't had much luck arguing this with our esteemed colleagues.
>
> In general, testing public interfaces is preferable, however, I think it's
> important to avoid becoming dogmatic. IMHO, it's more important to have tests
> that are clear in what they test than to not write tests (or write confusing
> tests) in order to adhere to a generalized principle.

That's a really good point.

> > So I think this is a very subtle point which is very widely
> > misunderstood. Most people write code and then write their tests,
> > following this practice along with only testing public interfaces
> > often causes people to just not test all of their code, which is
> > wrong.
>
> The very nature of this situation is that the code was written before the tests.
>
> > The idea of only testing public interfaces is supposed to make people
> > think more carefully about what the composite layers of the program
> > is. If you are having difficulty getting decent coverage by only
> > testing your public interfaces, then it likely tells you that you have
> > one of two problems:
> >
> > 1) You have code that you don't need, and you should remove it.
> >
> > 2) One of the layers in your program is too think, and you should
> > introduce a new layer with a new public interface that you can test
> > through.
> >
> > I think the second point here is problematic with how C is written in
> > the kernel. We don't really have any concept of public vs. private
> > inside the kernel outside of static vs. not static, which is much more
> > restricted.
>
> I don't think we can expect developers to refactor large portions of complex
> kernel code in order to improve its testability. I imagine this will happen
> naturally over time, but I think we need to allow for developers to test
> "private" code in the meanwhile.
>
> My opinion is that it's more important to have tests than not. As evidence, I
> submit the following commit:
> https://github.com/torvalds/linux/commit/156e42996bd84eccb6acf319f19ce0cb140d00e3.
>
> While not a major bug, this bug was discovered as a direct result of writing
> these unit tests. So, in summary, I see value in "testing the lowest level bits
> in isolation", even if it doesn't necessarily represent the Gold Standard in
> Unit Testing.

You're right.

I think, in summary, it seems that pretty much everyone agrees that we
need to provide a mechanism for testing low level bits of code in
isolation in such a way that the tests are easy to write, and don't
require the code under test to be massively refactored. Beyond that it
seems that we are mostly in between either including tests in the
source for the code under test or using the __visible_for_testing
mechanism. Between the two, it seems the preference is for including
the test in the source.

So I think I will still send out a patch to add in the
__visible_for_testing mechanism in case someone wants to use it in the
future.

Nevertheless, I will reformat this patch to include the tests in the
files that are under test. One question, do we have a preference
between putting all the tests in the same file as the code under test?
Or do we want to put them in a separate file that gets #included in
the file under test? I have a preference for the latter, but will
defer to what everyone else wants.

Thanks everyone!

^ permalink raw reply

* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-11-06  0:35 UTC (permalink / raw)
  To: Kees Cook
  Cc: Iurii Zaikin, shuah, John Johansen, jmorris, serge, Alan Maguire,
	David Gow, Luis Chamberlain, Theodore Ts'o,
	Linux Kernel Mailing List, linux-security-module,
	KUnit Development, open list:KERNEL SELFTEST FRAMEWORK,
	Mike Salvatore
In-Reply-To: <201910301157.58D0CE4D3@keescook>

On Wed, Oct 30, 2019 at 11:59 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
> > On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> >
> > > +config SECURITY_APPARMOR_TEST
> > > +       bool "Build KUnit tests for policy_unpack.c"
> > > +       default n
>
> New options already already default n, this can be left off.
>
> > > +       depends on KUNIT && SECURITY_APPARMOR
> > > +       help
> > >
> > select SECURITY_APPARMOR ?
>
> "select" doesn't enforce dependencies, so just a "depends ..." is
> correct.
>
> > > +       KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> > > +       KUNIT_EXPECT_TRUE(test,
> > > +               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > I think this must be  KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);,
> > otherwise there could be a buffer overflow in memcmp. All tests that
> > follow such pattern
>
> Agreed.
>
> > are suspect. Also, not sure about your stylistic preference for
> > KUNIT_EXPECT_TRUE(test,
> >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > vs
> > KUNIT_EXPECT_EQ(test,
> >                0,
> >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
>
> I like == 0.

Oh, I almost missed this. I think the *_EQ(...) is better than the
*_TRUE(...) because the EQ is able to provide more debug information
if the test fails (otherwise there would really be no point in
providing all these variants).

Any objections?

Thanks for the catch Iurii!

^ permalink raw reply

* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-11-06  0:37 UTC (permalink / raw)
  To: Kees Cook
  Cc: Iurii Zaikin, shuah, John Johansen, jmorris, serge, Alan Maguire,
	David Gow, Luis Chamberlain, Theodore Ts'o,
	Linux Kernel Mailing List, linux-security-module,
	KUnit Development, open list:KERNEL SELFTEST FRAMEWORK,
	Mike Salvatore
In-Reply-To: <CAFd5g47gfEJqRUW1PR1rtgrzekwLVqRRw0iJ4EVRW4xzUiW2Yw@mail.gmail.com>

On Tue, Nov 5, 2019 at 4:35 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Wed, Oct 30, 2019 at 11:59 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
> > > On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
> > > <brendanhiggins@google.com> wrote:
> > >
> > > > +config SECURITY_APPARMOR_TEST
> > > > +       bool "Build KUnit tests for policy_unpack.c"
> > > > +       default n
> >
> > New options already already default n, this can be left off.
> >
> > > > +       depends on KUNIT && SECURITY_APPARMOR
> > > > +       help
> > > >
> > > select SECURITY_APPARMOR ?
> >
> > "select" doesn't enforce dependencies, so just a "depends ..." is
> > correct.
> >
> > > > +       KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> > > > +       KUNIT_EXPECT_TRUE(test,
> > > > +               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > > I think this must be  KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);,
> > > otherwise there could be a buffer overflow in memcmp. All tests that
> > > follow such pattern
> >
> > Agreed.
> >
> > > are suspect. Also, not sure about your stylistic preference for
> > > KUNIT_EXPECT_TRUE(test,
> > >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > > vs
> > > KUNIT_EXPECT_EQ(test,
> > >                0,
> > >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
> >
> > I like == 0.
>
> Oh, I almost missed this. I think the *_EQ(...) is better than the
> *_TRUE(...) because the EQ is able to provide more debug information
> if the test fails (otherwise there would really be no point in
> providing all these variants).
>
> Any objections?
>
> Thanks for the catch Iurii!

Wait, nevermind.

Either way is fine because memcmp probably won't show terribly
interesting information in the non-zero case. I'll just leave it the
way Mike wrote it.

Sorry!

^ permalink raw reply

* [PATCH linux-kselftest/test v2] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-11-06  0:43 UTC (permalink / raw)
  To: shuah, john.johansen, jmorris, serge, keescook, alan.maguire,
	yzaikin, davidgow, mcgrof, tytso
  Cc: linux-kernel, linux-security-module, kunit-dev, linux-kselftest,
	Mike Salvatore, Brendan Higgins

From: Mike Salvatore <mike.salvatore@canonical.com>

Add KUnit tests to test AppArmor unpacking of userspace policies.
AppArmor uses a serialized binary format for loading policies. To find
policy format documentation see
Documentation/admin-guide/LSM/apparmor.rst.

In order to write the tests against the policy unpacking code, some
static functions needed to be exposed for testing purposes. One of the
goals of this patch is to establish a pattern for which testing these
kinds of functions should be done in the future.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Signed-off-by: Mike Salvatore <mike.salvatore@canonical.com>
---
 security/apparmor/Kconfig              |  16 +
 security/apparmor/policy_unpack.c      |   4 +
 security/apparmor/policy_unpack_test.c | 607 +++++++++++++++++++++++++
 3 files changed, 627 insertions(+)
 create mode 100644 security/apparmor/policy_unpack_test.c

diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
index d8b1a360a6368..78a33ccac2574 100644
--- a/security/apparmor/Kconfig
+++ b/security/apparmor/Kconfig
@@ -66,3 +66,19 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
 	  Set the default value of the apparmor.debug kernel parameter.
 	  When enabled, various debug messages will be logged to
 	  the kernel message buffer.
+
+config SECURITY_APPARMOR_KUNIT_TEST
+	bool "Build KUnit tests for policy_unpack.c"
+	depends on KUNIT && SECURITY_APPARMOR
+	help
+	  This builds the AppArmor KUnit tests.
+
+	  KUnit tests run during boot and output the results to the debug log
+	  in TAP format (http://testanything.org/). Only useful for kernel devs
+	  running KUnit test harness and are not for inclusion into a
+	  production build.
+
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 8cfc9493eefc7..37c1dd3178fc0 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -1120,3 +1120,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
 
 	return error;
 }
+
+#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
+#include "policy_unpack_test.c"
+#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
new file mode 100644
index 0000000000000..533137f45361c
--- /dev/null
+++ b/security/apparmor/policy_unpack_test.c
@@ -0,0 +1,607 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * KUnit tests for AppArmor's policy unpack.
+ */
+
+#include <kunit/test.h>
+
+#include "include/policy.h"
+#include "include/policy_unpack.h"
+
+#define TEST_STRING_NAME "TEST_STRING"
+#define TEST_STRING_DATA "testing"
+#define TEST_STRING_BUF_OFFSET \
+	(3 + strlen(TEST_STRING_NAME) + 1)
+
+#define TEST_U32_NAME "U32_TEST"
+#define TEST_U32_DATA ((u32)0x01020304)
+#define TEST_NAMED_U32_BUF_OFFSET \
+	(TEST_STRING_BUF_OFFSET + 3 + strlen(TEST_STRING_DATA) + 1)
+#define TEST_U32_BUF_OFFSET \
+	(TEST_NAMED_U32_BUF_OFFSET + 3 + strlen(TEST_U32_NAME) + 1)
+
+#define TEST_U16_OFFSET (TEST_U32_BUF_OFFSET + 3)
+#define TEST_U16_DATA ((u16)(TEST_U32_DATA >> 16))
+
+#define TEST_U64_NAME "U64_TEST"
+#define TEST_U64_DATA ((u64)0x0102030405060708)
+#define TEST_NAMED_U64_BUF_OFFSET (TEST_U32_BUF_OFFSET + sizeof(u32) + 1)
+#define TEST_U64_BUF_OFFSET \
+	(TEST_NAMED_U64_BUF_OFFSET + 3 + strlen(TEST_U64_NAME) + 1)
+
+#define TEST_BLOB_NAME "BLOB_TEST"
+#define TEST_BLOB_DATA "\xde\xad\x00\xbe\xef"
+#define TEST_BLOB_DATA_SIZE (ARRAY_SIZE(TEST_BLOB_DATA))
+#define TEST_NAMED_BLOB_BUF_OFFSET (TEST_U64_BUF_OFFSET + sizeof(u64) + 1)
+#define TEST_BLOB_BUF_OFFSET \
+	(TEST_NAMED_BLOB_BUF_OFFSET + 3 + strlen(TEST_BLOB_NAME) + 1)
+
+#define TEST_ARRAY_NAME "ARRAY_TEST"
+#define TEST_ARRAY_SIZE 16
+#define TEST_NAMED_ARRAY_BUF_OFFSET \
+	(TEST_BLOB_BUF_OFFSET + 5 + TEST_BLOB_DATA_SIZE)
+#define TEST_ARRAY_BUF_OFFSET \
+	(TEST_NAMED_ARRAY_BUF_OFFSET + 3 + strlen(TEST_ARRAY_NAME) + 1)
+
+struct policy_unpack_fixture {
+	struct aa_ext *e;
+	size_t e_size;
+};
+
+struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
+				   struct kunit *test, size_t buf_size)
+{
+	char *buf;
+	struct aa_ext *e;
+
+	buf = kunit_kzalloc(test, buf_size, GFP_USER);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, buf);
+
+	e = kunit_kmalloc(test, sizeof(*e), GFP_USER);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, e);
+
+	e->start = buf;
+	e->end = e->start + buf_size;
+	e->pos = e->start;
+
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_STRING_NAME) + 1;
+	strcpy(buf + 3, TEST_STRING_NAME);
+
+	buf = e->start + TEST_STRING_BUF_OFFSET;
+	*buf = AA_STRING;
+	*(buf + 1) = strlen(TEST_STRING_DATA) + 1;
+	strcpy(buf + 3, TEST_STRING_DATA);
+
+	buf = e->start + TEST_NAMED_U32_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_U32_NAME) + 1;
+	strcpy(buf + 3, TEST_U32_NAME);
+	*(buf + 3 + strlen(TEST_U32_NAME) + 1) = AA_U32;
+	*((u32 *)(buf + 3 + strlen(TEST_U32_NAME) + 2)) = TEST_U32_DATA;
+
+	buf = e->start + TEST_NAMED_U64_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_U64_NAME) + 1;
+	strcpy(buf + 3, TEST_U64_NAME);
+	*(buf + 3 + strlen(TEST_U64_NAME) + 1) = AA_U64;
+	*((u64 *)(buf + 3 + strlen(TEST_U64_NAME) + 2)) = TEST_U64_DATA;
+
+	buf = e->start + TEST_NAMED_BLOB_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_BLOB_NAME) + 1;
+	strcpy(buf + 3, TEST_BLOB_NAME);
+	*(buf + 3 + strlen(TEST_BLOB_NAME) + 1) = AA_BLOB;
+	*(buf + 3 + strlen(TEST_BLOB_NAME) + 2) = TEST_BLOB_DATA_SIZE;
+	memcpy(buf + 3 + strlen(TEST_BLOB_NAME) + 6,
+		TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE);
+
+	buf = e->start + TEST_NAMED_ARRAY_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_ARRAY_NAME) + 1;
+	strcpy(buf + 3, TEST_ARRAY_NAME);
+	*(buf + 3 + strlen(TEST_ARRAY_NAME) + 1) = AA_ARRAY;
+	*((u16 *)(buf + 3 + strlen(TEST_ARRAY_NAME) + 2)) = TEST_ARRAY_SIZE;
+
+	return e;
+}
+
+static int policy_unpack_test_init(struct kunit *test)
+{
+	size_t e_size = TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1;
+	struct policy_unpack_fixture *puf;
+
+	puf = kunit_kmalloc(test, sizeof(*puf), GFP_USER);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, puf);
+
+	puf->e_size = e_size;
+	puf->e = build_aa_ext_struct(puf, test, e_size);
+
+	test->priv = puf;
+	return 0;
+}
+
+static void policy_unpack_test_inbounds_when_inbounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+
+	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, 0));
+	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size / 2));
+	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size));
+}
+
+static void policy_unpack_test_inbounds_when_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+
+	KUNIT_EXPECT_FALSE(test, inbounds(puf->e, puf->e_size + 1));
+}
+
+static void policy_unpack_test_unpack_array_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	u16 array_size;
+
+	puf->e->pos += TEST_ARRAY_BUF_OFFSET;
+
+	array_size = unpack_array(puf->e, NULL);
+
+	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
+}
+
+static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_ARRAY_NAME;
+	u16 array_size;
+
+	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
+
+	array_size = unpack_array(puf->e, name);
+
+	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
+}
+
+static void policy_unpack_test_unpack_array_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_ARRAY_NAME;
+	u16 array_size;
+
+	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
+	puf->e->end = puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16);
+
+	array_size = unpack_array(puf->e, name);
+
+	KUNIT_EXPECT_EQ(test, array_size, (u16)0);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+		puf->e->start + TEST_NAMED_ARRAY_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_blob_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *blob = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_BLOB_BUF_OFFSET;
+	size = unpack_blob(puf->e, &blob, NULL);
+
+	KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);
+	KUNIT_EXPECT_TRUE(test,
+		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
+}
+
+static void policy_unpack_test_unpack_blob_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *blob = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
+	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
+
+	KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);
+	KUNIT_EXPECT_TRUE(test,
+		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
+}
+
+static void policy_unpack_test_unpack_blob_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *blob = NULL;
+	void *start;
+	int size;
+
+	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
+	start = puf->e->pos;
+	puf->e->end = puf->e->start + TEST_BLOB_BUF_OFFSET
+		+ TEST_BLOB_DATA_SIZE - 1;
+
+	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, 0);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+
+static void policy_unpack_test_unpack_str_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char *string = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_STRING_BUF_OFFSET;
+	size = unpack_str(puf->e, &string, NULL);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_str_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char *string = NULL;
+	size_t size;
+
+	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_str_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char *string = NULL;
+	void *start = puf->e->pos;
+	int size;
+
+	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
+		+ strlen(TEST_STRING_DATA) - 1;
+
+	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, 0);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+
+static void policy_unpack_test_unpack_strdup_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *string = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_STRING_BUF_OFFSET;
+	size = unpack_strdup(puf->e, &string, NULL);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_FALSE(test,
+			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
+			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_strdup_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *string = NULL;
+	size_t size;
+
+	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_FALSE(test,
+			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
+			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_strdup_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	void *start = puf->e->pos;
+	char *string = NULL;
+	int size;
+
+	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
+		+ strlen(TEST_STRING_DATA) - 1;
+
+	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, 0);
+	KUNIT_EXPECT_PTR_EQ(test, string, (char *)NULL);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+
+static void policy_unpack_test_unpack_nameX_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+
+	puf->e->pos += TEST_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_U32, NULL);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
+}
+
+static void policy_unpack_test_unpack_nameX_with_wrong_code(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+
+	puf->e->pos += TEST_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_BLOB, NULL);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_U32_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_nameX_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U32_NAME;
+	bool success;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_U32, name);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
+}
+
+static void policy_unpack_test_unpack_nameX_with_wrong_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	static const char name[] = "12345678";
+	bool success;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_U32, name);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_u16_chunk_basic(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *chunk = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_U16_OFFSET;
+	/*
+	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
+	 * the end of the allocated memory. Doing anything other than comparing
+	 * memory addresses is dangerous.
+	 */
+	puf->e->end += TEST_U16_DATA;
+
+	size = unpack_u16_chunk(puf->e, &chunk);
+
+	KUNIT_EXPECT_PTR_EQ(test, (void *)chunk,
+			    puf->e->start + TEST_U16_OFFSET + 2);
+	KUNIT_EXPECT_EQ(test, size, (size_t)TEST_U16_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, (void *)(chunk + TEST_U16_DATA));
+}
+
+static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_1(
+		struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *chunk = NULL;
+	size_t size;
+
+	puf->e->pos = puf->e->end - 1;
+
+	size = unpack_u16_chunk(puf->e, &chunk);
+
+	KUNIT_EXPECT_EQ(test, size, (size_t)0);
+	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->end - 1);
+}
+
+static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_2(
+		struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *chunk = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_U16_OFFSET;
+	/*
+	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
+	 * the end of the allocated memory. Doing anything other than comparing
+	 * memory addresses is dangerous.
+	 */
+	puf->e->end = puf->e->pos + TEST_U16_DATA - 1;
+
+	size = unpack_u16_chunk(puf->e, &chunk);
+
+	KUNIT_EXPECT_EQ(test, size, (size_t)0);
+	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->start + TEST_U16_OFFSET);
+}
+
+static void policy_unpack_test_unpack_u32_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+	u32 data;
+
+	puf->e->pos += TEST_U32_BUF_OFFSET;
+
+	success = unpack_u32(puf->e, &data, NULL);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
+}
+
+static void policy_unpack_test_unpack_u32_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U32_NAME;
+	bool success;
+	u32 data;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+
+	success = unpack_u32(puf->e, &data, name);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
+}
+
+static void policy_unpack_test_unpack_u32_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U32_NAME;
+	bool success;
+	u32 data;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+	puf->e->end = puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32);
+
+	success = unpack_u32(puf->e, &data, name);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_u64_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+	u64 data;
+
+	puf->e->pos += TEST_U64_BUF_OFFSET;
+
+	success = unpack_u64(puf->e, &data, NULL);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
+}
+
+static void policy_unpack_test_unpack_u64_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U64_NAME;
+	bool success;
+	u64 data;
+
+	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
+
+	success = unpack_u64(puf->e, &data, name);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
+}
+
+static void policy_unpack_test_unpack_u64_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U64_NAME;
+	bool success;
+	u64 data;
+
+	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
+	puf->e->end = puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64);
+
+	success = unpack_u64(puf->e, &data, name);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_NAMED_U64_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_X_code_match(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success = unpack_X(puf->e, AA_NAME);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start + 1);
+}
+
+static void policy_unpack_test_unpack_X_code_mismatch(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success = unpack_X(puf->e, AA_STRING);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start);
+}
+
+static void policy_unpack_test_unpack_X_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+
+	puf->e->pos = puf->e->end;
+	success = unpack_X(puf->e, AA_NAME);
+
+	KUNIT_EXPECT_FALSE(test, success);
+}
+
+static struct kunit_case apparmor_policy_unpack_test_cases[] = {
+	KUNIT_CASE(policy_unpack_test_inbounds_when_inbounds),
+	KUNIT_CASE(policy_unpack_test_inbounds_when_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_array_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_array_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_array_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_blob_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_blob_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_blob_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_code),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_name),
+	KUNIT_CASE(policy_unpack_test_unpack_str_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_str_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_str_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_strdup_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_basic),
+	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_1),
+	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_2),
+	KUNIT_CASE(policy_unpack_test_unpack_u32_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u32_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u32_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_u64_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u64_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u64_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_X_code_match),
+	KUNIT_CASE(policy_unpack_test_unpack_X_code_mismatch),
+	KUNIT_CASE(policy_unpack_test_unpack_X_out_of_bounds),
+	{},
+};
+
+static struct kunit_suite apparmor_policy_unpack_test_module = {
+	.name = "apparmor_policy_unpack",
+	.init = policy_unpack_test_init,
+	.test_cases = apparmor_policy_unpack_test_cases,
+};
+
+kunit_test_suite(apparmor_policy_unpack_test_module);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog


^ permalink raw reply related

* Re: [PATCH linux-kselftest/test v2] apparmor: add AppArmor KUnit tests for policy unpack
From: John Johansen @ 2019-11-06  8:05 UTC (permalink / raw)
  To: Brendan Higgins, shuah, jmorris, serge, keescook, alan.maguire,
	yzaikin, davidgow, mcgrof, tytso
  Cc: linux-kernel, linux-security-module, kunit-dev, linux-kselftest,
	Mike Salvatore
In-Reply-To: <20191106004329.16991-1-brendanhiggins@google.com>

On 11/5/19 4:43 PM, Brendan Higgins wrote:
> From: Mike Salvatore <mike.salvatore@canonical.com>
> 
> Add KUnit tests to test AppArmor unpacking of userspace policies.
> AppArmor uses a serialized binary format for loading policies. To find
> policy format documentation see
> Documentation/admin-guide/LSM/apparmor.rst.
> 
> In order to write the tests against the policy unpacking code, some
> static functions needed to be exposed for testing purposes. One of the
> goals of this patch is to establish a pattern for which testing these
> kinds of functions should be done in the future.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Signed-off-by: Mike Salvatore <mike.salvatore@canonical.com>

LGTM

Acked-by: John Johansen <john.johansen@canonical.com>


> ---
>  security/apparmor/Kconfig              |  16 +
>  security/apparmor/policy_unpack.c      |   4 +
>  security/apparmor/policy_unpack_test.c | 607 +++++++++++++++++++++++++
>  3 files changed, 627 insertions(+)
>  create mode 100644 security/apparmor/policy_unpack_test.c
> 
> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> index d8b1a360a6368..78a33ccac2574 100644
> --- a/security/apparmor/Kconfig
> +++ b/security/apparmor/Kconfig
> @@ -66,3 +66,19 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
>  	  Set the default value of the apparmor.debug kernel parameter.
>  	  When enabled, various debug messages will be logged to
>  	  the kernel message buffer.
> +
> +config SECURITY_APPARMOR_KUNIT_TEST
> +	bool "Build KUnit tests for policy_unpack.c"
> +	depends on KUNIT && SECURITY_APPARMOR
> +	help
> +	  This builds the AppArmor KUnit tests.
> +
> +	  KUnit tests run during boot and output the results to the debug log
> +	  in TAP format (http://testanything.org/). Only useful for kernel devs
> +	  running KUnit test harness and are not for inclusion into a
> +	  production build.
> +
> +	  For more information on KUnit and unit tests in general please refer
> +	  to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +	  If unsure, say N.
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 8cfc9493eefc7..37c1dd3178fc0 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -1120,3 +1120,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
>  
>  	return error;
>  }
> +
> +#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
> +#include "policy_unpack_test.c"
> +#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
> diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
> new file mode 100644
> index 0000000000000..533137f45361c
> --- /dev/null
> +++ b/security/apparmor/policy_unpack_test.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * KUnit tests for AppArmor's policy unpack.
> + */
> +
> +#include <kunit/test.h>
> +
> +#include "include/policy.h"
> +#include "include/policy_unpack.h"
> +
> +#define TEST_STRING_NAME "TEST_STRING"
> +#define TEST_STRING_DATA "testing"
> +#define TEST_STRING_BUF_OFFSET \
> +	(3 + strlen(TEST_STRING_NAME) + 1)
> +
> +#define TEST_U32_NAME "U32_TEST"
> +#define TEST_U32_DATA ((u32)0x01020304)
> +#define TEST_NAMED_U32_BUF_OFFSET \
> +	(TEST_STRING_BUF_OFFSET + 3 + strlen(TEST_STRING_DATA) + 1)
> +#define TEST_U32_BUF_OFFSET \
> +	(TEST_NAMED_U32_BUF_OFFSET + 3 + strlen(TEST_U32_NAME) + 1)
> +
> +#define TEST_U16_OFFSET (TEST_U32_BUF_OFFSET + 3)
> +#define TEST_U16_DATA ((u16)(TEST_U32_DATA >> 16))
> +
> +#define TEST_U64_NAME "U64_TEST"
> +#define TEST_U64_DATA ((u64)0x0102030405060708)
> +#define TEST_NAMED_U64_BUF_OFFSET (TEST_U32_BUF_OFFSET + sizeof(u32) + 1)
> +#define TEST_U64_BUF_OFFSET \
> +	(TEST_NAMED_U64_BUF_OFFSET + 3 + strlen(TEST_U64_NAME) + 1)
> +
> +#define TEST_BLOB_NAME "BLOB_TEST"
> +#define TEST_BLOB_DATA "\xde\xad\x00\xbe\xef"
> +#define TEST_BLOB_DATA_SIZE (ARRAY_SIZE(TEST_BLOB_DATA))
> +#define TEST_NAMED_BLOB_BUF_OFFSET (TEST_U64_BUF_OFFSET + sizeof(u64) + 1)
> +#define TEST_BLOB_BUF_OFFSET \
> +	(TEST_NAMED_BLOB_BUF_OFFSET + 3 + strlen(TEST_BLOB_NAME) + 1)
> +
> +#define TEST_ARRAY_NAME "ARRAY_TEST"
> +#define TEST_ARRAY_SIZE 16
> +#define TEST_NAMED_ARRAY_BUF_OFFSET \
> +	(TEST_BLOB_BUF_OFFSET + 5 + TEST_BLOB_DATA_SIZE)
> +#define TEST_ARRAY_BUF_OFFSET \
> +	(TEST_NAMED_ARRAY_BUF_OFFSET + 3 + strlen(TEST_ARRAY_NAME) + 1)
> +
> +struct policy_unpack_fixture {
> +	struct aa_ext *e;
> +	size_t e_size;
> +};
> +
> +struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
> +				   struct kunit *test, size_t buf_size)
> +{
> +	char *buf;
> +	struct aa_ext *e;
> +
> +	buf = kunit_kzalloc(test, buf_size, GFP_USER);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, buf);
> +
> +	e = kunit_kmalloc(test, sizeof(*e), GFP_USER);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, e);
> +
> +	e->start = buf;
> +	e->end = e->start + buf_size;
> +	e->pos = e->start;
> +
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_STRING_NAME) + 1;
> +	strcpy(buf + 3, TEST_STRING_NAME);
> +
> +	buf = e->start + TEST_STRING_BUF_OFFSET;
> +	*buf = AA_STRING;
> +	*(buf + 1) = strlen(TEST_STRING_DATA) + 1;
> +	strcpy(buf + 3, TEST_STRING_DATA);
> +
> +	buf = e->start + TEST_NAMED_U32_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_U32_NAME) + 1;
> +	strcpy(buf + 3, TEST_U32_NAME);
> +	*(buf + 3 + strlen(TEST_U32_NAME) + 1) = AA_U32;
> +	*((u32 *)(buf + 3 + strlen(TEST_U32_NAME) + 2)) = TEST_U32_DATA;
> +
> +	buf = e->start + TEST_NAMED_U64_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_U64_NAME) + 1;
> +	strcpy(buf + 3, TEST_U64_NAME);
> +	*(buf + 3 + strlen(TEST_U64_NAME) + 1) = AA_U64;
> +	*((u64 *)(buf + 3 + strlen(TEST_U64_NAME) + 2)) = TEST_U64_DATA;
> +
> +	buf = e->start + TEST_NAMED_BLOB_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_BLOB_NAME) + 1;
> +	strcpy(buf + 3, TEST_BLOB_NAME);
> +	*(buf + 3 + strlen(TEST_BLOB_NAME) + 1) = AA_BLOB;
> +	*(buf + 3 + strlen(TEST_BLOB_NAME) + 2) = TEST_BLOB_DATA_SIZE;
> +	memcpy(buf + 3 + strlen(TEST_BLOB_NAME) + 6,
> +		TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE);
> +
> +	buf = e->start + TEST_NAMED_ARRAY_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_ARRAY_NAME) + 1;
> +	strcpy(buf + 3, TEST_ARRAY_NAME);
> +	*(buf + 3 + strlen(TEST_ARRAY_NAME) + 1) = AA_ARRAY;
> +	*((u16 *)(buf + 3 + strlen(TEST_ARRAY_NAME) + 2)) = TEST_ARRAY_SIZE;
> +
> +	return e;
> +}
> +
> +static int policy_unpack_test_init(struct kunit *test)
> +{
> +	size_t e_size = TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1;
> +	struct policy_unpack_fixture *puf;
> +
> +	puf = kunit_kmalloc(test, sizeof(*puf), GFP_USER);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, puf);
> +
> +	puf->e_size = e_size;
> +	puf->e = build_aa_ext_struct(puf, test, e_size);
> +
> +	test->priv = puf;
> +	return 0;
> +}
> +
> +static void policy_unpack_test_inbounds_when_inbounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +
> +	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, 0));
> +	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size / 2));
> +	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size));
> +}
> +
> +static void policy_unpack_test_inbounds_when_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +
> +	KUNIT_EXPECT_FALSE(test, inbounds(puf->e, puf->e_size + 1));
> +}
> +
> +static void policy_unpack_test_unpack_array_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	u16 array_size;
> +
> +	puf->e->pos += TEST_ARRAY_BUF_OFFSET;
> +
> +	array_size = unpack_array(puf->e, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_ARRAY_NAME;
> +	u16 array_size;
> +
> +	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
> +
> +	array_size = unpack_array(puf->e, name);
> +
> +	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_array_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_ARRAY_NAME;
> +	u16 array_size;
> +
> +	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
> +	puf->e->end = puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16);
> +
> +	array_size = unpack_array(puf->e, name);
> +
> +	KUNIT_EXPECT_EQ(test, array_size, (u16)0);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +		puf->e->start + TEST_NAMED_ARRAY_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_blob_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *blob = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_BLOB_BUF_OFFSET;
> +	size = unpack_blob(puf->e, &blob, NULL);
> +
> +	KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> +	KUNIT_EXPECT_TRUE(test,
> +		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> +}
> +
> +static void policy_unpack_test_unpack_blob_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *blob = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
> +	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
> +
> +	KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> +	KUNIT_EXPECT_TRUE(test,
> +		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> +}
> +
> +static void policy_unpack_test_unpack_blob_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *blob = NULL;
> +	void *start;
> +	int size;
> +
> +	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
> +	start = puf->e->pos;
> +	puf->e->end = puf->e->start + TEST_BLOB_BUF_OFFSET
> +		+ TEST_BLOB_DATA_SIZE - 1;
> +
> +	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, 0);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
> +}
> +
> +static void policy_unpack_test_unpack_str_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char *string = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_STRING_BUF_OFFSET;
> +	size = unpack_str(puf->e, &string, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_str_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char *string = NULL;
> +	size_t size;
> +
> +	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_str_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char *string = NULL;
> +	void *start = puf->e->pos;
> +	int size;
> +
> +	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
> +		+ strlen(TEST_STRING_DATA) - 1;
> +
> +	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, 0);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
> +}
> +
> +static void policy_unpack_test_unpack_strdup_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *string = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_STRING_BUF_OFFSET;
> +	size = unpack_strdup(puf->e, &string, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_FALSE(test,
> +			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
> +			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_strdup_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *string = NULL;
> +	size_t size;
> +
> +	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_FALSE(test,
> +			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
> +			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_strdup_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	void *start = puf->e->pos;
> +	char *string = NULL;
> +	int size;
> +
> +	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
> +		+ strlen(TEST_STRING_DATA) - 1;
> +
> +	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, 0);
> +	KUNIT_EXPECT_PTR_EQ(test, string, (char *)NULL);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +
> +	puf->e->pos += TEST_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_U32, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_wrong_code(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +
> +	puf->e->pos += TEST_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_BLOB, NULL);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_U32_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U32_NAME;
> +	bool success;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_U32, name);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_wrong_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	static const char name[] = "12345678";
> +	bool success;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_U32, name);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_u16_chunk_basic(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *chunk = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_U16_OFFSET;
> +	/*
> +	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
> +	 * the end of the allocated memory. Doing anything other than comparing
> +	 * memory addresses is dangerous.
> +	 */
> +	puf->e->end += TEST_U16_DATA;
> +
> +	size = unpack_u16_chunk(puf->e, &chunk);
> +
> +	KUNIT_EXPECT_PTR_EQ(test, (void *)chunk,
> +			    puf->e->start + TEST_U16_OFFSET + 2);
> +	KUNIT_EXPECT_EQ(test, size, (size_t)TEST_U16_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, (void *)(chunk + TEST_U16_DATA));
> +}
> +
> +static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_1(
> +		struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *chunk = NULL;
> +	size_t size;
> +
> +	puf->e->pos = puf->e->end - 1;
> +
> +	size = unpack_u16_chunk(puf->e, &chunk);
> +
> +	KUNIT_EXPECT_EQ(test, size, (size_t)0);
> +	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->end - 1);
> +}
> +
> +static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_2(
> +		struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *chunk = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_U16_OFFSET;
> +	/*
> +	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
> +	 * the end of the allocated memory. Doing anything other than comparing
> +	 * memory addresses is dangerous.
> +	 */
> +	puf->e->end = puf->e->pos + TEST_U16_DATA - 1;
> +
> +	size = unpack_u16_chunk(puf->e, &chunk);
> +
> +	KUNIT_EXPECT_EQ(test, size, (size_t)0);
> +	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->start + TEST_U16_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_u32_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +	u32 data;
> +
> +	puf->e->pos += TEST_U32_BUF_OFFSET;
> +
> +	success = unpack_u32(puf->e, &data, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u32_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U32_NAME;
> +	bool success;
> +	u32 data;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +
> +	success = unpack_u32(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u32_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U32_NAME;
> +	bool success;
> +	u32 data;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +	puf->e->end = puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32);
> +
> +	success = unpack_u32(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_u64_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +	u64 data;
> +
> +	puf->e->pos += TEST_U64_BUF_OFFSET;
> +
> +	success = unpack_u64(puf->e, &data, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u64_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U64_NAME;
> +	bool success;
> +	u64 data;
> +
> +	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
> +
> +	success = unpack_u64(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u64_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U64_NAME;
> +	bool success;
> +	u64 data;
> +
> +	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
> +	puf->e->end = puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64);
> +
> +	success = unpack_u64(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_NAMED_U64_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_X_code_match(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success = unpack_X(puf->e, AA_NAME);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start + 1);
> +}
> +
> +static void policy_unpack_test_unpack_X_code_mismatch(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success = unpack_X(puf->e, AA_STRING);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start);
> +}
> +
> +static void policy_unpack_test_unpack_X_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +
> +	puf->e->pos = puf->e->end;
> +	success = unpack_X(puf->e, AA_NAME);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +}
> +
> +static struct kunit_case apparmor_policy_unpack_test_cases[] = {
> +	KUNIT_CASE(policy_unpack_test_inbounds_when_inbounds),
> +	KUNIT_CASE(policy_unpack_test_inbounds_when_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_array_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_array_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_array_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_blob_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_blob_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_blob_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_code),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_str_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_str_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_str_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_strdup_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_basic),
> +	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_1),
> +	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_2),
> +	KUNIT_CASE(policy_unpack_test_unpack_u32_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u32_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u32_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_u64_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u64_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u64_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_X_code_match),
> +	KUNIT_CASE(policy_unpack_test_unpack_X_code_mismatch),
> +	KUNIT_CASE(policy_unpack_test_unpack_X_out_of_bounds),
> +	{},
> +};
> +
> +static struct kunit_suite apparmor_policy_unpack_test_module = {
> +	.name = "apparmor_policy_unpack",
> +	.init = policy_unpack_test_init,
> +	.test_cases = apparmor_policy_unpack_test_cases,
> +};
> +
> +kunit_test_suite(apparmor_policy_unpack_test_module);
> 


^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: KP Singh @ 2019-11-06 10:06 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Mickaël Salaün, linux-kernel, Alexei Starovoitov,
	Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
	Florent Revest, James Morris, Jann Horn, John Johansen,
	Jonathan Corbet, Kees Cook, Michael Kerrisk,
	Mickaël Salaün, Paul Moore, Sargun Dhillon,
	Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
	Tetsuo Handa, Tycho Andersen, Will Drewry, bpf, kernel-hardening,
	linux-api, linux-security-module, netdev
In-Reply-To: <20191105193446.s4pswwwhrmgk6hcx@ast-mbp.dhcp.thefacebook.com>

On 05-Nov 11:34, Alexei Starovoitov wrote:
> On Tue, Nov 05, 2019 at 07:01:41PM +0100, Mickaël Salaün wrote:
> > 
> > On 05/11/2019 18:18, Alexei Starovoitov wrote:
> > > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> > >> Add a first Landlock hook that can be used to enforce a security policy
> > >> or to audit some process activities.  For a sandboxing use-case, it is
> > >> needed to inform the kernel if a task can legitimately debug another.
> > >> ptrace(2) can also be used by an attacker to impersonate another task
> > >> and remain undetected while performing malicious activities.
> > >>
> > >> Using ptrace(2) and related features on a target process can lead to a
> > >> privilege escalation.  A sandboxed task must then be able to tell the
> > >> kernel if another task is more privileged, via ptrace_may_access().
> > >>
> > >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ...
> > >> +static int check_ptrace(struct landlock_domain *domain,
> > >> +		struct task_struct *tracer, struct task_struct *tracee)
> > >> +{
> > >> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
> > >> +		.prog_ctx = {
> > >> +			.tracer = (uintptr_t)tracer,
> > >> +			.tracee = (uintptr_t)tracee,
> > >> +		},
> > >> +	};
> > > 
> > > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > > yet claiming that the end goal is to make landlock unprivileged?!
> > > The most basic security hole in the tool that is aiming to provide security.
> > 
> > How could you used these pointers without dedicated BPF helpers? This
> > context items are typed as PTR_TO_TASK and can't be used without a
> > dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
> > arithmetic is explicitly forbidden (and I added tests for that). Did I
> > miss something?
> 
> It's a pointer leak.
> 
> > 
> > > 
> > > I think the only way bpf-based LSM can land is both landlock and KRSI
> > > developers work together on a design that solves all use cases.
> > 
> > As I said in a previous cover letter [1], that would be great. I think
> > that the current Landlock bases (almost everything from this series
> > except the seccomp interface) should meet both needs, but I would like
> > to have the point of view of the KRSI developers.
> > 
> > [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
> > 
> > > BPF is capable
> > > to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> > > are custom solutions to specific security concerns. BPF subsystem was extended
> > > with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> > > program types with a lot of overlapping functionality. We couldn't figure out
> > > how to generalize them into single 'networking' program. Now we can and we
> > > should. Accepting two partially overlapping bpf-based LSMs would be repeating
> > > the same mistake again.
> > 
> > I'll let the LSM maintainers comment on whether BPF could be a superset
> > of all LSM, but given the complexity of an access-control system, I have
> > some doubts though. Anyway, we need to start somewhere and then iterate.
> > This patch series is a first step.
> 
> I would like KRSI folks to speak up. So far I don't see any sharing happening
> between landlock and KRSI. You're claiming this set is a first step. They're
> claiming the same about their patches. I'd like to set a patchset that was
> jointly developed.

We are willing to collaborate with the Landlock developers and come up
with a common approach that would work for Landlock and KRSI. I want
to mention that this collaboration and the current Landlock approach
of using an eBPF based LSM for unprivileged sandboxing only makes sense
if unprivileged usage of eBPF is going to be ever allowed.

Purely from a technical standpoint, both the current designs for
Landlock and KRSI target separate use cases and it would not be
possible to build "one on top of the other". We've tried to identify
the lowest denominator ("eBPF+LSM") requirements for both Landlock
(unprivileged sandboxing / Discretionary Access Control) and KRSI
(flexibility and unification of privileged MAC and Audit) and
prototyped an implementation based on the newly added / upcoming
features in BPF.

We've been successfully able to prototype the use cases for KRSI
(privileged MAC and Audit) using this "eBPF+LSM" and shared our
approach at the Linux Security Summit [1]:

* Use the new in-kernel BTF (CO-RE eBPF programs) [2] and the ability
  of the BPF verifier to use the BTF information for access validation
  to provide a more generic way to attach to the various LSM hooks.
  This potentially saves a lot of redundant work:

   - Creation of new program types.
   - Multiple types of contexts (or a single context with Unions).
   - Changes to the verifier and creation of new BPF argument types 
     (eg. PTR_TO_TASK)

* These new BPF features also alleviate the original concerns that we
  raised when initially proposing KRSI and designing for precise BPF
  helpers. We have some patches coming up which incorporate these new
  changes and will be sharing something on the mailing list after some
  cleanup.

We can use the common "eBPF+LSM" for both privileged MAC and Audit and
unprivileged sandboxing i.e. Discretionary Access Control.
Here's what it could look like:

* Common infrastructure allows attachment to all hooks which works well
  for privileged MAC and Audit. This could be extended to provide
  another attachment type for unprivileged DAC, which can restrict the
  hooks that can be attached to, and also the information that is
  exposed to the eBPF programs which is something that Landlock could
  build.

* This attachment could use the proposed landlock domains and attach to
  the task_struct providing the discretionary access control semantics.

[1] https://static.sched.com/hosted_files/lsseu2019/a2/Kernel%20Runtime%20Security%20Instrumentation.pdf
[2] http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf

- KP Singh

> 

^ permalink raw reply

* Re: [PATCH bpf-next v13 4/7] landlock: Add ptrace LSM hooks
From: KP Singh @ 2019-11-06 10:15 UTC (permalink / raw)
  To: Mickaël Salaün
  Cc: Alexei Starovoitov, linux-kernel, Alexei Starovoitov,
	Andy Lutomirski, Casey Schaufler, Daniel Borkmann, David Drysdale,
	Florent Revest, James Morris, Jann Horn, John Johansen,
	Jonathan Corbet, Kees Cook, Michael Kerrisk,
	Mickaël Salaün, Paul Moore, Sargun Dhillon,
	Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
	Tetsuo Handa, Tycho Andersen, Will Drewry, bpf, kernel-hardening,
	linux-api, linux-security-module, netdev
In-Reply-To: <23acf523-dbc4-855b-ca49-2bbfa5e7117e@digikod.net>

On 05-Nov 19:01, Mickaël Salaün wrote:
> 
> On 05/11/2019 18:18, Alexei Starovoitov wrote:
> > On Mon, Nov 04, 2019 at 06:21:43PM +0100, Mickaël Salaün wrote:
> >> Add a first Landlock hook that can be used to enforce a security policy
> >> or to audit some process activities.  For a sandboxing use-case, it is
> >> needed to inform the kernel if a task can legitimately debug another.
> >> ptrace(2) can also be used by an attacker to impersonate another task
> >> and remain undetected while performing malicious activities.
> >>
> >> Using ptrace(2) and related features on a target process can lead to a
> >> privilege escalation.  A sandboxed task must then be able to tell the
> >> kernel if another task is more privileged, via ptrace_may_access().
> >>
> >> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > ...
> >> +static int check_ptrace(struct landlock_domain *domain,
> >> +		struct task_struct *tracer, struct task_struct *tracee)
> >> +{
> >> +	struct landlock_hook_ctx_ptrace ctx_ptrace = {
> >> +		.prog_ctx = {
> >> +			.tracer = (uintptr_t)tracer,
> >> +			.tracee = (uintptr_t)tracee,
> >> +		},
> >> +	};
> > 
> > So you're passing two kernel pointers obfuscated as u64 into bpf program
> > yet claiming that the end goal is to make landlock unprivileged?!
> > The most basic security hole in the tool that is aiming to provide security.
> 
> How could you used these pointers without dedicated BPF helpers? This
> context items are typed as PTR_TO_TASK and can't be used without a
> dedicated helper able to deal with ARG_PTR_TO_TASK. Moreover, pointer
> arithmetic is explicitly forbidden (and I added tests for that). Did I
> miss something?
> 
> > 
> > I think the only way bpf-based LSM can land is both landlock and KRSI
> > developers work together on a design that solves all use cases.
> 
> As I said in a previous cover letter [1], that would be great. I think
> that the current Landlock bases (almost everything from this series
> except the seccomp interface) should meet both needs, but I would like
> to have the point of view of the KRSI developers.

As I mentioned we are willing to collaborate but the current landlock
patches does not meet the needs for KRSI:

* One program type per use-case (eg. LANDLOCK_PROG_PTRACE) as opposed to
  a single program type. This is something that KRSI proposed in it's
  initial design [1] and the new common "eBPF + LSM" based approach
  [2] would maintain as well.

* Landlock chooses to have multiple LSM hooks per landlock hook which is
  more restrictive. It's not easy to write precise MAC and Audit
  policies for a privileged LSM based on this and this ends up bloating
  the context that needs to be maintained and requires avoidable
  boilerplate work in the kernel.

[1] https://lore.kernel.org/patchwork/project/lkml/list/?series=410101
[2] https://lore.kernel.org/bpf/20191106100655.GA18815@chromium.org/T/#u

- KP Singh

> 
> [1] https://lore.kernel.org/lkml/20191029171505.6650-1-mic@digikod.net/
> 
> > BPF is capable
> > to be a superset of all existing LSMs whereas landlock and KRSI propsals today
> > are custom solutions to specific security concerns. BPF subsystem was extended
> > with custom things in the past. In networking we have lwt, skb, tc, xdp, sk
> > program types with a lot of overlapping functionality. We couldn't figure out
> > how to generalize them into single 'networking' program. Now we can and we
> > should. Accepting two partially overlapping bpf-based LSMs would be repeating
> > the same mistake again.
> 
> I'll let the LSM maintainers comment on whether BPF could be a superset
> of all LSM, but given the complexity of an access-control system, I have
> some doubts though. Anyway, we need to start somewhere and then iterate.
> This patch series is a first step.

^ permalink raw reply

page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox