Re: [PATCH v19 06/23] LSM: Use lsmblob in security_secctx_to_secid
On 7/28/20 4:41 PM, Casey Schaufler wrote: > On 7/28/2020 4:11 AM, John Johansen wrote: >> On 7/24/20 1:32 PM, Casey Schaufler wrote: >>> Change security_secctx_to_secid() to fill in a lsmblob instead >>> of a u32 secid. Multiple LSMs may be able to interpret the >>> string, and this allows for setting whichever secid is >>> appropriate. Change security_secmark_relabel_packet() to use a >>> lsmblob instead of a u32 secid. In some other cases there is >>> scaffolding where interfaces have yet to be converted. >>> >>> Reviewed-by: Kees Cook >>> Signed-off-by: Casey Schaufler >>> Cc: netdev@vger.kernel.org >> one comment below, but its a nice to have so >> >> Reviewed-by: John Johansen >> >> >>> --- >>> include/linux/security.h | 30 +++ >>> include/net/scm.h | 7 +-- >>> kernel/cred.c | 4 +--- >>> net/ipv4/ip_sockglue.c| 6 -- >>> net/netfilter/nft_meta.c | 18 +--- >>> net/netfilter/xt_SECMARK.c| 9 ++-- >>> net/netlabel/netlabel_unlabeled.c | 23 + >>> security/security.c | 34 ++- >>> 8 files changed, 98 insertions(+), 33 deletions(-) >>> >>> diff --git a/include/linux/security.h b/include/linux/security.h >>> index d81e8886d799..98176faaaba5 100644 >>> --- a/include/linux/security.h >>> +++ b/include/linux/security.h >>> @@ -189,6 +189,27 @@ static inline bool lsmblob_equal(struct lsmblob >>> *bloba, struct lsmblob *blobb) >>> return !memcmp(bloba, blobb, sizeof(*bloba)); >>> } >>> >>> +/** >>> + * lsmblob_value - find the first non-zero value in an lsmblob structure. >>> + * @blob: Pointer to the data >>> + * >>> + * This needs to be used with extreme caution, as the cases where >>> + * it is appropriate are rare. >>> + * >>> + * Return the first secid value set in the lsmblob. >>> + * There should only be one. >> It would be really nice if we could have an LSM debug config, that would >> do things like checking there is indeed only one value when this fn >> is called. > > I can't see a CONFIG_LSM_DEBUG for this alone, but if you have > other places you'd like to see it I'm open to it. > yeah there are a few other places, this really isn't a requirement just a thought while I was going through these again. I will have to spend some time chasing them down. Maybe even cobble together a patch
Re: [PATCH v19 15/23] LSM: Use lsmcontext in security_secid_to_secctx
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext pointer to allow return of the LSM identifier
> along with the context and context length. This allows
> security_release_secctx() to know how to release the
> context. Callers have been modified to use or save the
> returned data from the new structure.
>
> Reviewed-by: Kees Cook
> Acked-by: Stephen Smalley
> Acked-by: Paul Moore
> Signed-off-by: Casey Schaufler
> Cc: netdev@vger.kernel.org
> ---
> drivers/android/binder.c| 26 +++-
> include/linux/security.h| 4 +--
> include/net/scm.h | 10 ++-
> kernel/audit.c | 35 --
> kernel/auditsc.c| 31 +++
> net/ipv4/ip_sockglue.c | 7 ++---
> net/netfilter/nf_conntrack_netlink.c| 18 +--
> net/netfilter/nf_conntrack_standalone.c | 7 ++---
> net/netfilter/nfnetlink_queue.c | 5 +++-
> net/netlabel/netlabel_unlabeled.c | 40 -
> net/netlabel/netlabel_user.c| 7 ++---
> security/security.c | 10 +--
> 12 files changed, 76 insertions(+), 124 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index b7ab206f8bb3..ceb5987c7d76 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2861,9 +2861,7 @@ static void binder_transaction(struct binder_proc *proc,
> binder_size_t last_fixup_min_off = 0;
> struct binder_context *context = proc->context;
> int t_debug_id = atomic_inc_return(&binder_last_id);
> - char *secctx = NULL;
> - u32 secctx_sz = 0;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext lsmctx = { };
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3111,14 +3109,14 @@ static void binder_transaction(struct binder_proc
> *proc,
> size_t added_size;
>
> security_task_getsecid(proc->tsk, &blob);
> - ret = security_secid_to_secctx(&blob, &secctx, &secctx_sz);
> + ret = security_secid_to_secctx(&blob, &lsmctx);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> return_error_line = __LINE__;
> goto err_get_secctx_failed;
> }
> - added_size = ALIGN(secctx_sz, sizeof(u64));
> + added_size = ALIGN(lsmctx.len, sizeof(u64));
> extra_buffers_size += added_size;
> if (extra_buffers_size < added_size) {
> /* integer overflow of extra_buffers_size */
> @@ -3145,24 +3143,22 @@ static void binder_transaction(struct binder_proc
> *proc,
> t->buffer = NULL;
> goto err_binder_alloc_buf_failed;
> }
> - if (secctx) {
> + if (lsmctx.context) {
> int err;
> size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
> ALIGN(tr->offsets_size, sizeof(void *)) +
> ALIGN(extra_buffers_size, sizeof(void *)) -
> - ALIGN(secctx_sz, sizeof(u64));
> + ALIGN(lsmctx.len, sizeof(u64));
>
> t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
> err = binder_alloc_copy_to_buffer(&target_proc->alloc,
> t->buffer, buf_offset,
> - secctx, secctx_sz);
> + lsmctx.context, lsmctx.len);
> if (err) {
> t->security_ctx = 0;
> WARN_ON(1);
> }
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - secctx = NULL;
> + security_release_secctx(&lsmctx);
> }
> t->buffer->debug_id = t->debug_id;
> t->buffer->transaction = t;
> @@ -3218,7 +3214,7 @@ static void binder_transaction(struct binder_proc *proc,
> off_end_offset = off_start_offset + tr->offsets_size;
> sg_buf_offset = ALIGN(off_end_offset, sizeof(void *));
> sg_buf_end_offset = sg_buf_offset + extra_buffers_size -
> - ALIGN(secctx_sz, sizeof(u64));
> + ALIGN(lsmctx.len, sizeof(u64));
> off_min = 0;
> for (buffer_offset = off_start_offset; buffer_offset < off_end_offset;
>buffer_offset += sizeof(binder_size_t)) {
> @@ -3494,10 +3490,8 @@ static void binder_transaction(struct binder_proc
> *proc,
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> err_bad_extra_size:
> - if (secctx) {
> -
Re: [PATCH v19 06/23] LSM: Use lsmblob in security_secctx_to_secid
On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Change security_secctx_to_secid() to fill in a lsmblob instead
> of a u32 secid. Multiple LSMs may be able to interpret the
> string, and this allows for setting whichever secid is
> appropriate. Change security_secmark_relabel_packet() to use a
> lsmblob instead of a u32 secid. In some other cases there is
> scaffolding where interfaces have yet to be converted.
>
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
> Cc: netdev@vger.kernel.org
one comment below, but its a nice to have so
Reviewed-by: John Johansen
> ---
> include/linux/security.h | 30 +++
> include/net/scm.h | 7 +--
> kernel/cred.c | 4 +---
> net/ipv4/ip_sockglue.c| 6 --
> net/netfilter/nft_meta.c | 18 +---
> net/netfilter/xt_SECMARK.c| 9 ++--
> net/netlabel/netlabel_unlabeled.c | 23 +
> security/security.c | 34 ++-
> 8 files changed, 98 insertions(+), 33 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d81e8886d799..98176faaaba5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -189,6 +189,27 @@ static inline bool lsmblob_equal(struct lsmblob *bloba,
> struct lsmblob *blobb)
> return !memcmp(bloba, blobb, sizeof(*bloba));
> }
>
> +/**
> + * lsmblob_value - find the first non-zero value in an lsmblob structure.
> + * @blob: Pointer to the data
> + *
> + * This needs to be used with extreme caution, as the cases where
> + * it is appropriate are rare.
> + *
> + * Return the first secid value set in the lsmblob.
> + * There should only be one.
It would be really nice if we could have an LSM debug config, that would
do things like checking there is indeed only one value when this fn
is called.
> + */
> +static inline u32 lsmblob_value(const struct lsmblob *blob)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (blob->secid[i])
> + return blob->secid[i];
> +
> + return 0;
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> @@ -502,7 +523,8 @@ int security_setprocattr(const char *lsm, const char
> *name, void *value,
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> + struct lsmblob *blob);
> void security_release_secctx(char *secdata, u32 seclen);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1321,7 +1343,7 @@ static inline int security_secid_to_secctx(u32 secid,
> char **secdata, u32 *secle
>
> static inline int security_secctx_to_secid(const char *secdata,
> u32 seclen,
> -u32 *secid)
> +struct lsmblob *blob)
> {
> return -EOPNOTSUPP;
> }
> @@ -1411,7 +1433,7 @@ void security_inet_csk_clone(struct sock *newsk,
> const struct request_sock *req);
> void security_inet_conn_established(struct sock *sk,
> struct sk_buff *skb);
> -int security_secmark_relabel_packet(u32 secid);
> +int security_secmark_relabel_packet(struct lsmblob *blob);
> void security_secmark_refcount_inc(void);
> void security_secmark_refcount_dec(void);
> int security_tun_dev_alloc_security(void **security);
> @@ -1584,7 +1606,7 @@ static inline void
> security_inet_conn_established(struct sock *sk,
> {
> }
>
> -static inline int security_secmark_relabel_packet(u32 secid)
> +static inline int security_secmark_relabel_packet(struct lsmblob *blob)
> {
> return 0;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index e2e71c4bf9d0..c09f2dfeec88 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -97,8 +97,11 @@ static inline void scm_passec(struct socket *sock, struct
> msghdr *msg, struct sc
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - /* Scaffolding - it has to be element 0 for now */
> - err = security_secid_to_secctx(scm->lsmblob.secid[0
Re: [PATCH v18 05/23] net: Prepare UDS for security module stacking
On 7/9/20 9:11 AM, Stephen Smalley wrote:
> On Wed, Jul 8, 2020 at 8:23 PM Casey Schaufler wrote:
>>
>> Change the data used in UDS SO_PEERSEC processing from a
>> secid to a more general struct lsmblob. Update the
>> security_socket_getpeersec_dgram() interface to use the
>> lsmblob. There is a small amount of scaffolding code
>> that will come out when the security_secid_to_secctx()
>> code is brought in line with the lsmblob.
>>
>> The secid field of the unix_skb_parms structure has been
>> replaced with a pointer to an lsmblob structure, and the
>> lsmblob is allocated as needed. This is similar to how the
>> list of passed files is managed. While an lsmblob structure
>> will fit in the available space today, there is no guarantee
>> that the addition of other data to the unix_skb_parms or
>> support for additional security modules wouldn't exceed what
>> is available.
>>
>> Reviewed-by: Kees Cook
>> Signed-off-by: Casey Schaufler
>> Cc: netdev@vger.kernel.org
>> ---
>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index 3385a7a0b231..d246aefcf4da 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -138,17 +138,23 @@ static struct hlist_head *unix_sockets_unbound(void
>> *addr)
>> #ifdef CONFIG_SECURITY_NETWORK
>> static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>> {
>> - UNIXCB(skb).secid = scm->secid;
>> + UNIXCB(skb).lsmdata = kmemdup(&scm->lsmblob, sizeof(scm->lsmblob),
>> + GFP_KERNEL);
>> }
>>
>> static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff
>> *skb)
>> {
>> - scm->secid = UNIXCB(skb).secid;
>> + if (likely(UNIXCB(skb).lsmdata))
>> + scm->lsmblob = *(UNIXCB(skb).lsmdata);
>> + else
>> + lsmblob_init(&scm->lsmblob, 0);
>> }
>>
>> static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff
>> *skb)
>> {
>> - return (scm->secid == UNIXCB(skb).secid);
>> + if (likely(UNIXCB(skb).lsmdata))
>> + return lsmblob_equal(&scm->lsmblob, UNIXCB(skb).lsmdata);
>> + return false;
>> }
>
> I don't think that this provides sensible behavior to userspace. On a
> transient memory allocation failure, instead of returning an error to
> the sender and letting them handle it, this will just proceed with
> sending the message without its associated security information, and
> potentially split messages on arbitrary boundaries because it cannot
> tell whether the sender had the same security information. I think
> you instead need to change unix_get_secdata() to return an error on
> allocation failure and propagate that up to the sender. Not a fan of
> this change in general both due to extra overhead on this code path
> and potential for breakage on allocation failures. I know it was
> motivated by paul's observation that we won't be able to fit many more
> secids into the cb but not sure we have to go there prematurely,
> especially absent its usage by upstream AA (no unix_stream_connect
> hook implementation upstream). Also not sure how the whole bpf local
I'm not sure how premature it is, I am running late for 5.9 but would
like to land apparmor unix mediation in 5.10
> storage approach to supporting security modules (or at least bpf lsm)
> might reduce need for expanding these structures?
>
Re: bpf: Massive skbuff_head_cache memory leak?
On 09/26/2018 02:22 PM, Daniel Borkmann wrote: > On 09/26/2018 11:09 PM, Tetsuo Handa wrote: >> Hello, Alexei and Daniel. >> >> Can you show us how to run testcases you are testing? > > Sorry for the delay; currently quite backlogged but will definitely take a > look > at these reports. Regarding your question: majority of test cases are in the > kernel tree under selftests, see tools/testing/selftests/bpf/ . > Its unlikely to be apparmor. I went through the reports and saw nothing that would indicate apparmor involvement, but the primary reason is what is being tested in upstream apparmor atm. The current upstream code does nothing directly with skbuffs. Its possible that the audit code paths (kernel audit does grab skbuffs) could, but there are only a couple cases that would be triggered in the current fuzzing so this seems to be an unlikely source for such a large leak. >> On 2018/09/22 22:25, Tetsuo Handa wrote: >>> Hello. >>> >>> syzbot is reporting many lockup problems on bpf.git / bpf-next.git / >>> net.git / net-next.git trees. >>> >>> INFO: rcu detected stall in br_multicast_port_group_expired (2) >>> >>> https://syzkaller.appspot.com/bug?id=15c7ad8cf35a07059e8a697a22527e11d294bc94 >>> >>> INFO: rcu detected stall in tun_chr_close >>> >>> https://syzkaller.appspot.com/bug?id=6c50618bde03e5a2eefdd0269cf9739c5ebb8270 >>> >>> INFO: rcu detected stall in discover_timer >>> >>> https://syzkaller.appspot.com/bug?id=55da031ddb910e58ab9c6853a5784efd94f03b54 >>> >>> INFO: rcu detected stall in ret_from_fork (2) >>> >>> https://syzkaller.appspot.com/bug?id=c83129a6683b44b39f5b8864a1325893c9218363 >>> >>> INFO: rcu detected stall in addrconf_rs_timer >>> >>> https://syzkaller.appspot.com/bug?id=21c029af65f81488edbc07a10ed20792444711b6 >>> >>> INFO: rcu detected stall in kthread (2) >>> >>> https://syzkaller.appspot.com/bug?id=6accd1ed11c31110fed1982f6ad38cc9676477d2 >>> >>> INFO: rcu detected stall in ext4_filemap_fault >>> >>> https://syzkaller.appspot.com/bug?id=817e38d20e9ee53390ac361bf0fd2007eaf188af >>> >>> INFO: rcu detected stall in run_timer_softirq (2) >>> >>> https://syzkaller.appspot.com/bug?id=f5a230a3ff7822f8d39fddf8485931bd06ae47fe >>> >>> INFO: rcu detected stall in bpf_prog_ADDR >>> >>> https://syzkaller.appspot.com/bug?id=fb4911fd0e861171cc55124e209f810a0dd68744 >>> >>> INFO: rcu detected stall in __run_timers (2) >>> >>> https://syzkaller.appspot.com/bug?id=65416569ddc8d2feb8f19066aa761f5a47f7451a >>> >>> The cause of lockup seems to be flood of printk() messages from memory >>> allocation >>> failures, and one of out_of_memory() messages indicates that >>> skbuff_head_cache >>> usage is huge enough to suspect in-kernel memory leaks. >>> >>> [ 1554.547011] skbuff_head_cache1847887KB1847887KB >>> >>> Unfortunately, we cannot find from logs what syzbot is trying to do >>> because constant printk() messages is flooding away syzkaller messages. >>> Can you try running your testcases with kmemleak enabled? >>> >> >> On 2018/09/27 2:35, Dmitry Vyukov wrote: >>> I also started suspecting Apparmor. We switched to Apparmor on Aug 30: >>> https://groups.google.com/d/msg/syzkaller-bugs/o73lO4KGh0w/j9pcH2tSBAAJ >>> Now the instances that use SELinux and Smack explicitly contain that >>> in the name, but the rest are Apparmor. >>> Aug 30 roughly matches these assorted "task hung" reports. Perhaps >>> some Apparmor hook leaks a reference to skbs? >> >> Maybe. They have CONFIG_DEFAULT_SECURITY="apparmor". But I'm wondering why >> this problem is not occurring on linux-next.git when this problem is >> occurring >> on bpf.git / bpf-next.git / net.git / net-next.git trees. Is syzbot running >> different testcases depending on which git tree is targeted? >> > this is another reason that it is doubtful that its apparmor.
Re: [PATCH 10/12] apparmorfs: Replace CURRENT_TIME with current_time()
On 04/07/2017 05:57 PM, Deepa Dinamani wrote: > CURRENT_TIME macro is not y2038 safe on 32 bit systems. > > The patch replaces all the uses of CURRENT_TIME by > current_time(). > > This is also in preparation for the patch that transitions > vfs timestamps to use 64 bit time and hence make them > y2038 safe. current_time() is also planned to be transitioned > to y2038 safe behavior along with this change. > > CURRENT_TIME macro will be deleted before merging the > aforementioned change. > > Signed-off-by: Deepa Dinamani This is all good, and I have no objections to it being merged for 4.12. If it isn't this change is already queued up for the apparmor 4.13 merge Acked-by: John Johansen > --- > security/apparmor/apparmorfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index be0b498..4f6ac9d 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -1357,7 +1357,7 @@ static int aa_mk_null_file(struct dentry *parent) > > inode->i_ino = get_next_ino(); > inode->i_mode = S_IFCHR | S_IRUGO | S_IWUGO; > - inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME; > + inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode); > init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO, > MKDEV(MEM_MAJOR, 3)); > d_instantiate(dentry, inode); >
