- Mar 01, 2023
-
-
Linus Torvalds authored
Back in 2008 we extended the capability bits from 32 to 64, and we did it by extending the single 32-bit capability word from one word to an array of two words. It was then obfuscated by hiding the "2" behind two macro expansions, with the reasoning being that maybe it gets extended further some day. That reasoning may have been valid at the time, but the last thing we want to do is to extend the capability set any more. And the array of values not only causes source code oddities (with loops to deal with it), but also results in worse code generation. It's a lose-lose situation. So just change the 'u32[2]' into a 'u64' and be done with it. We still have to deal with the fact that the user space interface is designed around an array of these 32-bit values, but that was the case before too, since the array layouts were different (ie user space doesn't use an array of 32-bit values for individual capability masks, but an array of 32-bit slices of multiple masks). So that marshalling of data is actually simplified too, even if it does remain somewhat obscure and odd. This was all triggered by my reaction to the new "cap_isidentical()" introduced recently. By just using a saner data structure, it went from unsigned __capi; CAP_FOR_EACH_U32(__capi) { if (a.cap[__capi] != b.cap[__capi]) return false; } return true; to just being return a.val == b.val; instead. Which is rather more obvious both to humans and to compilers. Cc: Mateusz Guzik <mjguzik@gmail.com> Cc: Casey Schaufler <casey@schaufler-ca.com> Cc: Serge Hallyn <serge@hallyn.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Paul Moore <paul@paul-moore.com> Signed-off-by:
Linus Torvalds <torvalds@linux-foundation.org>
-
- Feb 15, 2023
-
-
John Johansen authored
This fixes a regression in mediation of getattr when old policy built under an older ABI is loaded and mapped to internal permissions. The regression does not occur for all getattr permission requests, only appearing if state zero is the final state in the permission lookup. This is because despite the first state (index 0) being guaranteed to not have permissions in both newer and older permission formats, it may have to carry permissions that were not mediated as part of an older policy. These backward compat permissions are mapped here to avoid special casing the mediation code paths. Since the mapping code already takes into account backwards compat permission from older formats it can be applied to state 0 to fix the regression. Fixes: 408d53e9 ("apparmor: compute file permissions on profile load") Reported-by:
Philip Meulengracht <the_meulengracht@hotmail.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
- Jan 19, 2023
-
-
Christian Brauner authored
Convert to struct mnt_idmap. Remove legacy file_mnt_user_ns() and mnt_user_ns(). Last cycle we merged the necessary infrastructure in 256c8aed ("fs: introduce dedicated idmap type for mounts"). This is just the conversion to struct mnt_idmap. Currently we still pass around the plain namespace that was attached to a mount. This is in general pretty convenient but it makes it easy to conflate namespaces that are relevant on the filesystem with namespaces that are relevent on the mount level. Especially for non-vfs developers without detailed knowledge in this area this can be a potential source for bugs. Once the conversion to struct mnt_idmap is done all helpers down to the really low-level helpers will take a struct mnt_idmap argument instead of two namespace arguments. This way it becomes impossible to conflate the two eliminating the possibility of any bugs. All of the vfs and all filesystems only operate on struct mnt_idmap. Acked-by:
Dave Chinner <dchinner@redhat.com> Reviewed-by:
Christoph Hellwig <hch@lst.de> Signed-off-by:
Christian Brauner (Microsoft) <brauner@kernel.org>
-
Christian Brauner authored
Convert to struct mnt_idmap. Last cycle we merged the necessary infrastructure in 256c8aed ("fs: introduce dedicated idmap type for mounts"). This is just the conversion to struct mnt_idmap. Currently we still pass around the plain namespace that was attached to a mount. This is in general pretty convenient but it makes it easy to conflate namespaces that are relevant on the filesystem with namespaces that are relevent on the mount level. Especially for non-vfs developers without detailed knowledge in this area this can be a potential source for bugs. Once the conversion to struct mnt_idmap is done all helpers down to the really low-level helpers will take a struct mnt_idmap argument instead of two namespace arguments. This way it becomes impossible to conflate the two eliminating the possibility of any bugs. All of the vfs and all filesystems only operate on struct mnt_idmap. Acked-by:
Dave Chinner <dchinner@redhat.com> Reviewed-by:
Christoph Hellwig <hch@lst.de> Signed-off-by:
Christian Brauner (Microsoft) <brauner@kernel.org>
-
Christian Brauner authored
Convert to struct mnt_idmap. Last cycle we merged the necessary infrastructure in 256c8aed ("fs: introduce dedicated idmap type for mounts"). This is just the conversion to struct mnt_idmap. Currently we still pass around the plain namespace that was attached to a mount. This is in general pretty convenient but it makes it easy to conflate namespaces that are relevant on the filesystem with namespaces that are relevent on the mount level. Especially for non-vfs developers without detailed knowledge in this area this can be a potential source for bugs. Once the conversion to struct mnt_idmap is done all helpers down to the really low-level helpers will take a struct mnt_idmap argument instead of two namespace arguments. This way it becomes impossible to conflate the two eliminating the possibility of any bugs. All of the vfs and all filesystems only operate on struct mnt_idmap. Acked-by:
Dave Chinner <dchinner@redhat.com> Reviewed-by:
Christoph Hellwig <hch@lst.de> Signed-off-by:
Christian Brauner (Microsoft) <brauner@kernel.org>
-
Hao Sun authored
Similar to kmemdup(), but support large amount of bytes with kvmalloc() and does *not* guarantee that the result will be physically contiguous. Use only in cases where kvmalloc() is needed and free it with kvfree(). Also adapt policy_unpack.c in case someone bisect into this. Link: https://lkml.kernel.org/r/20221221144245.27164-1-sunhao.th@gmail.com Signed-off-by:
Hao Sun <sunhao.th@gmail.com> Suggested-by:
Daniel Borkmann <daniel@iogearbox.net> Cc: Nick Terrell <terrelln@fb.com> Cc: John Johansen <john.johansen@canonical.com> Cc: Paul Moore <paul@paul-moore.com> Cc: James Morris <jmorris@namei.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Signed-off-by:
Andrew Morton <akpm@linux-foundation.org>
-
- Dec 12, 2022
-
-
Rae Moar authored
Use macros, VISIBLE_IF_KUNIT and EXPORT_SYMBOL_IF_KUNIT, to allow static symbols to be conditionally set to be visible during apparmor_policy_unpack_test, which removes the need to include the testing file in the implementation file. Change the namespace of the symbols that are now conditionally visible (by adding the prefix aa_) to avoid confusion with symbols of the same name. Allow the test to be built as a module and namespace the module name from policy_unpack_test to apparmor_policy_unpack_test to improve clarity of the module name. Provide an example of how static symbols can be dealt with in testing. Signed-off-by:
Rae Moar <rmoar@google.com> Reviewed-by:
David Gow <davidgow@google.com> Acked-by:
John Johansen <john.johansen@canonical.com> Signed-off-by:
Shuah Khan <skhan@linuxfoundation.org>
-
- Nov 18, 2022
-
-
Paul Moore authored
The vfs_getxattr_alloc() function currently returns a ssize_t value despite the fact that it only uses int values internally for return values. Fix this by converting vfs_getxattr_alloc() to return an int type and adjust the callers as necessary. As part of these caller modifications, some of the callers are fixed to properly free the xattr value buffer on both success and failure to ensure that memory is not leaked in the failure case. Reviewed-by:
Serge Hallyn <serge@hallyn.com> Reviewed-by:
Mimi Zohar <zohar@linux.ibm.com> Signed-off-by:
Paul Moore <paul@paul-moore.com>
-
- Nov 05, 2022
-
-
Paul Moore authored
Commit 4ff09db1 ("bpf: net: Change sk_getsockopt() to take the sockptr_t argument") made it possible to call sk_getsockopt() with both user and kernel address space buffers through the use of the sockptr_t type. Unfortunately at the time of conversion the security_socket_getpeersec_stream() LSM hook was written to only accept userspace buffers, and in a desire to avoid having to change the LSM hook the commit author simply passed the sockptr_t's userspace buffer pointer. Since the only sk_getsockopt() callers at the time of conversion which used kernel sockptr_t buffers did not allow SO_PEERSEC, and hence the security_socket_getpeersec_stream() hook, this was acceptable but also very fragile as future changes presented the possibility of silently passing kernel space pointers to the LSM hook. There are several ways to protect against this, including careful code review of future commits, but since relying on code review to catch bugs is a recipe for disaster and the upstream eBPF maintainer is "strongly against defensive programming", this patch updates the LSM hook, and all of the implementations to support sockptr_t and safely handle both user and kernel space buffers. Acked-by:
Casey Schaufler <casey@schaufler-ca.com> Acked-by:
John Johansen <john.johansen@canonical.com> Signed-off-by:
Paul Moore <paul@paul-moore.com>
-
- Nov 02, 2022
-
-
John Johansen authored
Make sure array_size is initialized in the kunit test to get rid of compiler warnings. This will also make sure the following tests fail consistently if the first test fails. Reported-by:
kernel test robot <lkp@intel.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Xiu Jianfeng authored
The aa_setup_dfa_engine() and aa_teardown_dfa_engine() is only called in apparmor_init(), so let us add __init annotation to them. Fixes: 11c236b8 ("apparmor: add a default null dfa") Signed-off-by:
Xiu Jianfeng <xiujianfeng@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
- Nov 01, 2022
-
-
Xiu Jianfeng authored
After changes in commit a1bd627b ("apparmor: share profile name on replacement"), the hname member of struct aa_policy is not valid slab object, but a subset of that, it can not be freed by kfree_sensitive(), use aa_policy_destroy() to fix it. Fixes: a1bd627b ("apparmor: share profile name on replacement") Signed-off-by:
Xiu Jianfeng <xiujianfeng@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
- Oct 26, 2022
-
-
Christian Brauner authored
We already ported most parts and filesystems over for v6.0 to the new vfs{g,u}id_t type and associated helpers for v6.0. Convert the remaining places so we can remove all the old helpers. This is a non-functional change. Reviewed-by:
Seth Forshee (DigitalOcean) <sforshee@kernel.org> Acked-by:
John Johansen <john.johansen@canonical.com> Signed-off-by:
Christian Brauner (Microsoft) <brauner@kernel.org>
-
- Oct 25, 2022
-
-
Xiu Jianfeng authored
Before aa_alloc_profile(), it has allocated string for @*ns_name if @tmpns is not NULL, so directly return -ENOMEM if aa_alloc_profile() failed will cause a memleak issue, and even if aa_alloc_profile() succeed, in the @fail_profile tag of aa_unpack(), it need to free @ns_name as well, this patch fixes them. Fixes: 736ec752 ("AppArmor: policy routines for loading and unpacking policy") Fixes: 04dc715e ("apparmor: audit policy ns specified in policy load") Signed-off-by:
Xiu Jianfeng <xiujianfeng@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Gaosheng Cui authored
When the aa_profile is released, we will call free_ruleset to release aa_ruleset, but we don't free the memory of aa_ruleset, so there will be memleak, fix it. unreferenced object 0xffff8881475df800 (size 1024): comm "apparmor_parser", pid 883, jiffies 4294899650 (age 9114.088s) hex dump (first 32 bytes): 00 f8 5d 47 81 88 ff ff 00 f8 5d 47 81 88 ff ff ..]G......]G.... 00 00 00 00 00 00 00 00 00 dc 65 47 81 88 ff ff ..........eG.... backtrace: [<00000000370e658e>] __kmem_cache_alloc_node+0x182/0x700 [<00000000f2f5a6d2>] kmalloc_trace+0x2c/0x130 [<00000000c5c905b3>] aa_alloc_profile+0x1bc/0x5c0 [<00000000bc4fa72b>] unpack_profile+0x319/0x30c0 [<00000000eab791e9>] aa_unpack+0x307/0x1450 [<000000002c3a6ee1>] aa_replace_profiles+0x1b8/0x3790 [<00000000d0c3fd54>] policy_update+0x35a/0x890 [<00000000d04fed90>] profile_replace+0x1d1/0x260 [<00000000cba0c0a7>] vfs_write+0x283/0xd10 [<000000006bae64a5>] ksys_write+0x134/0x260 [<00000000b2fd8f31>] __x64_sys_write+0x78/0xb0 [<00000000f3c8a015>] do_syscall_64+0x5c/0x90 [<00000000a242b1db>] entry_SYSCALL_64_after_hwframe+0x63/0xcd Fixes: 217af7e2 ("apparmor: refactor profile rules and attachments") Signed-off-by:
Gaosheng Cui <cuigaosheng1@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Yang Li authored
'resouce' -> 'resource' Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=2396 Reported-by:
Abaci Robot <abaci@linux.alibaba.com> Signed-off-by:
Yang Li <yang.lee@linux.alibaba.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Xiu Jianfeng authored
According to the implementations of cred_label() and set_cred_label(), we should use pointer to struct aa_label for lbs_cred instead of struct aa_task_ctx, this patch fixes it. Fixes: bbd3662a ("Infrastructure management of the cred security blob") Signed-off-by:
Xiu Jianfeng <xiujianfeng@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Jiapeng Chong authored
security/apparmor/ipc.c:53: warning: expecting prototype for audit_cb(). Prototype was for audit_signal_cb() instead. Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=2337 Reported-by:
Abaci Robot <abaci@linux.alibaba.com> Signed-off-by:
Jiapeng Chong <jiapeng.chong@linux.alibaba.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Jiapeng Chong authored
security/apparmor/lsm.c:753: warning: expecting prototype for apparmor_bprm_committed_cred(). Prototype was for apparmor_bprm_committed_creds() instead. Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=2338 Reported-by:
Abaci Robot <abaci@linux.alibaba.com> Signed-off-by:
Jiapeng Chong <jiapeng.chong@linux.alibaba.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Jiapeng Chong authored
security/apparmor/audit.c:93: warning: expecting prototype for audit_base(). Prototype was for audit_pre() instead. Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=2339 Reported-by:
Abaci Robot <abaci@linux.alibaba.com> Signed-off-by:
Jiapeng Chong <jiapeng.chong@linux.alibaba.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
Unfortunately it is possible for some userspace's to load children profiles before the parent profile. This can even happen when the child and the parent are in different load sets. Fix this by creating a null place holder profile that grants no permissions and can be replaced by the parent once it is loaded. Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
Bother unconfined and learning profiles use the null profile as their base. Refactor so they are share a common base routine. This doesn't save much atm but will be important when the feature set of the parent is inherited. Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Gaosheng Cui authored
Update the comments for aa_getprocattr() and audit_resource(), the args of them have beed changed since commit 76a1d263 ("apparmor: switch getprocattr to using label_print fns()"). Signed-off-by:
Gaosheng Cui <cuigaosheng1@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Gaosheng Cui authored
Remove the following useless static inline functions: 1. label_is_visible() is a static function in security/apparmor/label.c, and it's not used, aa_ns_visible() can do the same things as it, so it's redundant. 2. is_deleted() is a static function in security/apparmor/file.c, and it's not used since commit aebd873e ("apparmor: refactor path name lookup and permission checks around labels"), so it's redundant. They are redundant, so remove them. Signed-off-by:
Gaosheng Cui <cuigaosheng1@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
- Oct 19, 2022
-
-
Günther Noack authored
Like path_truncate, the file_truncate hook also restricts file truncation, but is called in the cases where truncation is attempted on an already-opened file. This is required in a subsequent commit to handle ftruncate() operations differently to truncate() operations. Acked-by:
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Acked-by:
John Johansen <john.johansen@canonical.com> Acked-by:
Paul Moore <paul@paul-moore.com> Signed-off-by:
Günther Noack <gnoack3000@gmail.com> Link: https://lore.kernel.org/r/20221018182216.301684-2-gnoack3000@gmail.com Signed-off-by:
Mickaël Salaün <mic@digikod.net>
-
- Oct 11, 2022
-
-
John Johansen authored
unpack_profile() sets a default error on entry but this gets overridden by error assignment by functions called in its body. If an error check that was relying on the default value is triggered after one of these error assignments then zero will be passed to ERR_PTR. Fix this by setting up a default -EPROTO assignment in the error path and while we are at it make sure the correct error is returned in non-default cases. Fixes: 217af7e2 ("apparmor: refactor profile rules and attachments") Reported-by:
kernel test robot <lkp@intel.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
- Oct 10, 2022
-
-
John Johansen authored
The error path has one case where *table is uninitialized, initialize it. Fixes: a0792e2c ("apparmor: make transition table unpack generic so it can be reused") Reported-by:
kernel test robot <lkp@intel.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
- Oct 04, 2022
-
-
Muhammad Usama Anjum authored
The unpack_perms_table() can return error which is negative value. Store the return value to a signed variable. policy->size is unsigned variable. It shouldn't be used to store the return status. Fixes: 2d6b2dea7f3c ("apparmor: add the ability for policy to specify a permission table") Signed-off-by:
Muhammad Usama Anjum <usama.anjum@collabora.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
- Oct 03, 2022
-
-
John Johansen authored
The apparmor kunit tests are failing on the out of bounds array check with the following failure # policy_unpack_test_unpack_array_out_of_bounds: EXPECTATION FAILED at security/apparmor/policy_unpack_test.c:178 Expected unpack_array(puf->e, name, &array_size) == 1, but unpack_array(puf->e, name, &array_size) == -1 # policy_unpack_test_unpack_array_out_of_bounds: EXPECTATION FAILED at security/apparmor/policy_unpack_test.c:180 Expected array_size == 0, but array_size == 64192 not ok 5 - policy_unpack_test_unpack_array_out_of_bounds This is because unpack_array changed to allow distinguishing between the array not being present and an error. In the error case the array size is not set and should not be tested. Reported-by:
kernel test robot <yujie.liu@intel.com> Fixes: 995a5b64620e ("apparmor: make unpack_array return a trianary value") Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
The rawdata readback has a few of problems. First if compression is enabled when the data is read then the compressed data is read out instead decompressing the data. Second if compression of the data fails, the code does not handle holding onto the raw_data in uncompressed form. Third if the compression is enabled/disabled after the rawdata was loaded, the check against the global control of whether to use compression does not reflect what was already done to the data. Fix these by always storing the compressed size, along with the original data size even if compression fails or is not used. And use this to detect whether the rawdata is actually compressed. Fixes: 52ccc20c652b ("apparmor: use zstd compression for profile data") Signed-off-by:
John Johansen <john.johansen@canonical.com> Acked-by:
Jon Tourville <jon.tourville@canonical.com>
-
John Johansen authored
Unfortunately the switch to using zstd compression did not properly ifdef all the code that uses zstd_ symbols. So that if exporting of binary policy is disabled in the config the compile will fail with the following errors security/apparmor/lsm.c:1545: undefined reference to `zstd_min_clevel' aarch64-linux-ld: security/apparmor/lsm.c:1545: undefined reference to `zstd_max_clevel' Reported-by:
kernel test robot <lkp@intel.com> Fixes: 52ccc20c652b ("apparmor: use zstd compression for profile data") Signed-off-by:
John Johansen <john.johansen@canonical.com> Acked-by:
Jon Tourville <jon.tourville@canonical.com>
-
John Johansen authored
The decompress ctx was not properly initialized when reading raw profile data back to userspace. Reported-by:
kernel test robot <lkp@intel.com> Fixes: 52ccc20c652b ("apparmor: use zstd compression for profile data") Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
The index into the trans_table has a max size of 2^24 bits which the code was testing but this is unnecessary as unpack_array can only unpack a table of 2^16 bits in size so the table unpacked will never be larger than what can be indexed, and any test here is redundant. Reported-by:
kernel test robot <lkp@intel.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
When compute_fperms was moved to policy_compat and made static it was renamed from aa_compute_fperms to just compute_fperms to help indicate it is only available statically. Unfortunately the doc comment did not also get updated to reflect the change. Reported-by:
kernel test robot <lkp@intel.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Xiu Jianfeng authored
Make __aa_path_perm() static as it's only used inside apparmor/file.c. Signed-off-by:
Xiu Jianfeng <xiujianfeng@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
Gaosheng Cui authored
In aa_get_task_label(), aa_get_newest_cred_label(__task_cred(task)) can do the same things as aa_get_newest_label(__aa_task_raw_label(task)), so we can replace it and remove __aa_task_raw_label() to simplify the code. Signed-off-by:
Gaosheng Cui <cuigaosheng1@huawei.com> Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
The unconfined label flag is not being computed correctly. It should only be set if all the profiles in the vector are set, which is different than what is required for the debug and stale flag that are set if any on the profile flags are set. Fixes: c1ed5da1 ("apparmor: allow label to carry debug flags") Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
The class name map did not have the reserved names added. Fix this Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
Convert profile->rules to a list as the next step towards supporting multiple rulesets in a profile. For this step only support a single list entry item. The logic for iterating the list will come as a separate step. Signed-off-by:
John Johansen <john.johansen@canonical.com>
-
John Johansen authored
In preparation for moving from a single set of rules and a single attachment to multiple rulesets and attachments separate from the profile refactor attachment information and ruleset info into their own structures. Signed-off-by:
John Johansen <john.johansen@canonical.com>
-