On Thu, Jun 27, 2019 at 09:29:25AM -0700, Casey Schaufler wrote:
> On 6/26/2019 8:53 PM, Kees Cook wrote:
> > On Wed, Jun 26, 2019 at 12:22:28PM -0700, 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.
> >>
> >> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> >> ---
> >> drivers/android/binder.c | 24 ++++++---------
> >> include/linux/security.h | 4 +--
> >> include/net/scm.h | 9 ++----
> >> kernel/audit.c | 29 +++++++-----------
> >> kernel/auditsc.c | 31 +++++++------------
> >> net/ipv4/ip_sockglue.c | 7 ++---
> >> net/netfilter/nf_conntrack_netlink.c | 14 +++++----
> >> 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 | 9 ++++--
> >> 12 files changed, 72 insertions(+), 114 deletions(-)
> >>
> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >> index 89e574be34cc..5d417a7b9bb3 100644
> >> --- a/drivers/android/binder.c
> >> +++ b/drivers/android/binder.c
> >> @@ -2874,9 +2874,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;
> > As James found, this needs to be zero initialized:
> >
> > struct lsmcontext lsmctx = { };
>
> Thanks! I'll incorporate this in v5. It's great to
> have y'all checking it out.
I looked through other removed NULL assignments, and I think I see some
other issues...
binder_alloc_copy_to_buffer(&target_proc->alloc,
t->buffer, buf_offset,
- secctx, secctx_sz);
- security_release_secctx(secctx, secctx_sz);
- secctx = NULL;
+ lsmctx.context, lsmctx.len);
+ security_release_secctx(&lsmctx);
The new security_release_secctx() performs the zeroing if there is a
slot match... should it be unconditional? (And should the no-op version
also zero?)
@@ -2420,8 +2420,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct
svc_fh *fhp,
__be32 status;
int err;
struct nfs4_acl *acl = NULL;
- void *context = NULL;
- int contextlen;
+ struct lsmcontext context;
This needs the same = { } to retain the same meaning.
And here:
@@ -1191,8 +1191,8 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh)
struct audit_buffer *ab;
u16 msg_type = nlh->nlmsg_type;
struct audit_sig_info *sig_data;
- char *ctx = NULL;
u32 len;
+ struct lsmcontext context;
And here:
@@ -2069,24 +2070,23 @@ void audit_log_key(struct audit_buffer *ab, char
*key)
int audit_log_task_context(struct audit_buffer *ab)
{
- char *ctx = NULL;
- unsigned len;
int error;
- u32 sid;
+ struct lsmblob le;
+ struct lsmcontext context;
and here:
static int audit_log_pid_context(struct audit_context *context, pid_t
pid,
- kuid_t auid, kuid_t uid, unsigned int
sessionid,
- u32 sid, char *comm)
+ kuid_t auid, kuid_t uid,
+ unsigned int sessionid,
+ struct lsmblob *l, char *comm)
{
struct audit_buffer *ab;
- char *ctx = NULL;
- u32 len;
+ struct lsmcontext lsmctx;
Maybe here?
- if (osid) {
- char *ctx = NULL;
- u32 len;
- if (security_secid_to_secctx(osid, &ctx, &len))
{
- audit_log_format(ab, " osid=%u", osid);
+ if (lsmblob_is_set(olsm)) {
+ struct lsmcontext lsmcxt;
+ if (security_secid_to_secctx(olsm, &lsmcxt))
*call_panic = 1;
and:
- if (n->osid != 0) {
- char *ctx = NULL;
- u32 len;
+ if (lsmblob_is_set(&n->olsm)) {
+ struct lsmcontext lsmctx;
and:
@@ -395,7 +401,7 @@ nfqnl_build_packet_message(struct net *net, struct
nfqnl_instance *queue,
enum ip_conntrack_info uninitialized_var(ctinfo);
struct nfnl_ct_hook *nfnl_ct;
bool csum_verify;
- char *secdata = NULL;
+ struct lsmcontext context;
and:
@@ -387,8 +387,7 @@ int netlbl_unlhsh_add(struct net *net,
struct net_device *dev;
struct netlbl_unlhsh_iface *iface;
struct audit_buffer *audit_buf = NULL;
- char *secctx = NULL;
- u32 secctx_len;
+ struct lsmcontext context;
Sorry I forgot to check those the first time through!
--
Kees Cook
