* Re: WARNING: refcount bug in find_key_to_update
From: David Howells @ 2019-10-18 16:45 UTC (permalink / raw)
To: Tetsuo Handa
Cc: dhowells, Eric Biggers, Linus Torvalds, syzbot, keyrings,
LSM List, syzkaller-bugs
In-Reply-To: <b211005b-75de-7936-c97a-817f7100415a@I-love.SAKURA.ne.jp>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> I don't know about keys, but I rather suspect lack of serialization locks
> between "looking up for checking existing keys" versus "looking up for
> garbage collection".
The garbage collector holds key_serial_lock when walking key_serial_tree
looking for keys to destroy.
As the gc is the *only* thing that is permitted to remove a key from
key_serial_tree, it can safely keep a cursor pointer to the node it was
looking at when it drops the lock - and then resume scanning once it has taken
the lock again.
When find_key_to_update() is looking for a key that might be updated, the
caller *must* be holding the destination keyring lock and every key in the
keyring should have at least one ref on it held by the keyring - so none of
them should get destroyed by the garbage collector.
David
^ permalink raw reply
* [PATCH] kernel: convert switch/case fallthrough comments to fallthrough;
From: Joe Perches @ 2019-10-18 16:35 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-audit, linux-kernel, netdev, bpf, linux-security-module,
kgdb-bugreport, linux-pm, linux-block
Use the new pseudo keyword "fallthrough;" and not the
various /* fallthrough */ style comments.
Signed-off-by: Joe Perches <joe@perches.com>
---
This is a single patch for the kernel/ source tree,
which would otherwise
be sent through as separate
patches to 19 maintainer sections.
compilation tested only.
Done by the script in this email:
https://lore.kernel.org/lkml/9fe980f7e28242c2835ffae34914c5f68e8268a7.camel@perches.com/
kernel/auditfilter.c | 2 +-
kernel/bpf/cgroup.c | 4 ++--
kernel/bpf/verifier.c | 4 ++--
kernel/capability.c | 2 +-
kernel/compat.c | 6 +++---
kernel/debug/gdbstub.c | 6 +++---
kernel/debug/kdb/kdb_keyboard.c | 4 ++--
kernel/debug/kdb/kdb_support.c | 6 +++---
kernel/events/core.c | 3 +--
kernel/futex.c | 4 ++--
kernel/gcov/gcc_3_4.c | 6 +++---
kernel/irq/handle.c | 3 +--
kernel/irq/manage.c | 5 ++---
kernel/kallsyms.c | 4 ++--
kernel/pid.c | 2 +-
kernel/power/hibernate.c | 2 +-
kernel/power/qos.c | 4 ++--
kernel/printk/printk.c | 2 +-
kernel/sched/core.c | 2 +-
kernel/sched/topology.c | 6 +++---
kernel/signal.c | 2 +-
kernel/sys.c | 3 +--
kernel/time/hrtimer.c | 2 +-
kernel/time/posix-timers.c | 4 ++--
kernel/time/tick-broadcast.c | 2 +-
kernel/time/timer.c | 2 +-
kernel/trace/blktrace.c | 2 +-
kernel/trace/trace_events_filter.c | 4 ++--
28 files changed, 47 insertions(+), 51 deletions(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index b0126e9c0743..471cd680479d 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -674,7 +674,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
data->values[i] = AUDIT_UID_UNSET;
break;
}
- /* fall through - if set */
+ fallthrough; /* if set */
default:
data->values[i] = f->val;
}
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index ddd8addcdb5c..955631f1b77d 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -797,7 +797,7 @@ cgroup_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_trace_printk:
if (capable(CAP_SYS_ADMIN))
return bpf_get_trace_printk_proto();
- /* fall through */
+ fallthrough;
default:
return NULL;
}
@@ -1439,7 +1439,7 @@ static bool cg_sockopt_is_valid_access(int off, int size,
return prog->expected_attach_type ==
BPF_CGROUP_GETSOCKOPT;
case offsetof(struct bpf_sockopt, optname):
- /* fallthrough */
+ fallthrough;
case offsetof(struct bpf_sockopt, level):
if (size != size_default)
return false;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ffc3e53f5300..d2b6fd8545e2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2249,7 +2249,7 @@ static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
case BPF_PROG_TYPE_CGROUP_SKB:
if (t == BPF_WRITE)
return false;
- /* fallthrough */
+ fallthrough;
/* Program types with direct read + write access go here! */
case BPF_PROG_TYPE_SCHED_CLS:
@@ -4381,7 +4381,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
off_reg == dst_reg ? dst : src);
return -EACCES;
}
- /* fall-through */
+ fallthrough;
default:
break;
}
diff --git a/kernel/capability.c b/kernel/capability.c
index 1444f3954d75..7c59b096c98a 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -93,7 +93,7 @@ static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
break;
case _LINUX_CAPABILITY_VERSION_2:
warn_deprecated_v2();
- /* fall through - v3 is otherwise equivalent to v2. */
+ fallthrough; /* v3 is otherwise equivalent to v2 */
case _LINUX_CAPABILITY_VERSION_3:
*tocopy = _LINUX_CAPABILITY_U32S_3;
break;
diff --git a/kernel/compat.c b/kernel/compat.c
index a2bc1d6ceb57..d9c61f4317be 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -343,11 +343,11 @@ get_compat_sigset(sigset_t *set, const compat_sigset_t __user *compat)
return -EFAULT;
switch (_NSIG_WORDS) {
case 4: set->sig[3] = v.sig[6] | (((long)v.sig[7]) << 32 );
- /* fall through */
+ fallthrough;
case 3: set->sig[2] = v.sig[4] | (((long)v.sig[5]) << 32 );
- /* fall through */
+ fallthrough;
case 2: set->sig[1] = v.sig[2] | (((long)v.sig[3]) << 32 );
- /* fall through */
+ fallthrough;
case 1: set->sig[0] = v.sig[0] | (((long)v.sig[1]) << 32 );
}
#else
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index 4b280fc7dd67..b9d8b7248964 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1033,14 +1033,14 @@ int gdb_serial_stub(struct kgdb_state *ks)
return DBG_PASS_EVENT;
}
#endif
- /* Fall through */
+ fallthrough;
case 'C': /* Exception passing */
tmp = gdb_cmd_exception_pass(ks);
if (tmp > 0)
goto default_handle;
if (tmp == 0)
break;
- /* Fall through - on tmp < 0 */
+ fallthrough; /* on tmp < 0 */
case 'c': /* Continue packet */
case 's': /* Single step packet */
if (kgdb_contthread && kgdb_contthread != current) {
@@ -1049,7 +1049,7 @@ int gdb_serial_stub(struct kgdb_state *ks)
break;
}
dbg_activate_sw_breakpoints();
- /* Fall through - to default processing */
+ fallthrough; /* to default processing */
default:
default_handle:
error = kgdb_arch_handle_exception(ks->ex_vector,
diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
index 750497b0003a..f877a0a0d7cf 100644
--- a/kernel/debug/kdb/kdb_keyboard.c
+++ b/kernel/debug/kdb/kdb_keyboard.c
@@ -173,11 +173,11 @@ int kdb_get_kbd_char(void)
case KT_LATIN:
if (isprint(keychar))
break; /* printable characters */
- /* fall through */
+ fallthrough;
case KT_SPEC:
if (keychar == K_ENTER)
break;
- /* fall through */
+ fallthrough;
default:
return -1; /* ignore unprintables */
}
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index b8e6306e7e13..d636506f695a 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -432,7 +432,7 @@ int kdb_getphysword(unsigned long *word, unsigned long addr, size_t size)
*word = w8;
break;
}
- /* fall through */
+ fallthrough;
default:
diag = KDB_BADWIDTH;
kdb_printf("kdb_getphysword: bad width %ld\n", (long) size);
@@ -481,7 +481,7 @@ int kdb_getword(unsigned long *word, unsigned long addr, size_t size)
*word = w8;
break;
}
- /* fall through */
+ fallthrough;
default:
diag = KDB_BADWIDTH;
kdb_printf("kdb_getword: bad width %ld\n", (long) size);
@@ -525,7 +525,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
diag = kdb_putarea(addr, w8);
break;
}
- /* fall through */
+ fallthrough;
default:
diag = KDB_BADWIDTH;
kdb_printf("kdb_putword: bad width %ld\n", (long) size);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9ec0b0bfddbd..04e75b1144c5 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9361,8 +9361,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
case IF_SRC_KERNELADDR:
case IF_SRC_KERNEL:
kernel = 1;
- /* fall through */
-
+ fallthrough;
case IF_SRC_FILEADDR:
case IF_SRC_FILE:
if (state != IF_STATE_SOURCE)
diff --git a/kernel/futex.c b/kernel/futex.c
index bd18f60e4c6c..ab12b6229d2d 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3639,12 +3639,12 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
switch (cmd) {
case FUTEX_WAIT:
val3 = FUTEX_BITSET_MATCH_ANY;
- /* fall through */
+ fallthrough;
case FUTEX_WAIT_BITSET:
return futex_wait(uaddr, flags, val, timeout, val3);
case FUTEX_WAKE:
val3 = FUTEX_BITSET_MATCH_ANY;
- /* fall through */
+ fallthrough;
case FUTEX_WAKE_BITSET:
return futex_wake(uaddr, flags, val, val3);
case FUTEX_REQUEUE:
diff --git a/kernel/gcov/gcc_3_4.c b/kernel/gcov/gcc_3_4.c
index 801ee4b0b969..32fc3278166f 100644
--- a/kernel/gcov/gcc_3_4.c
+++ b/kernel/gcov/gcc_3_4.c
@@ -455,7 +455,7 @@ int gcov_iter_next(struct gcov_iterator *iter)
case RECORD_COUNT:
/* Advance to next count */
iter->count++;
- /* fall through */
+ fallthrough;
case RECORD_COUNT_LEN:
if (iter->count < get_func(iter)->n_ctrs[iter->type]) {
iter->record = 9;
@@ -465,7 +465,7 @@ int gcov_iter_next(struct gcov_iterator *iter)
get_type(iter)->offset += iter->count;
iter->count = 0;
iter->type++;
- /* fall through */
+ fallthrough;
case RECORD_FUNCTION_CHECK:
if (iter->type < iter->num_types) {
iter->record = 7;
@@ -474,7 +474,7 @@ int gcov_iter_next(struct gcov_iterator *iter)
/* Advance to next function */
iter->type = 0;
iter->function++;
- /* fall through */
+ fallthrough;
case RECORD_TIME_STAMP:
if (iter->function < iter->info->n_functions)
iter->record = 3;
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index a4ace611f47f..b38d2fd70fe1 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -165,8 +165,7 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags
}
__irq_wake_thread(desc, action);
-
- /* Fall through - to add to randomness */
+ fallthrough; /* to add to randomness */
case IRQ_HANDLED:
*flags |= action->flags;
break;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 1753486b440c..baa86020f243 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -222,7 +222,7 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
case IRQ_SET_MASK_OK:
case IRQ_SET_MASK_OK_DONE:
cpumask_copy(desc->irq_common_data.affinity, mask);
- /* fall through */
+ fallthrough;
case IRQ_SET_MASK_OK_NOCOPY:
irq_validate_effective_affinity(data);
irq_set_thread_affinity(desc);
@@ -792,8 +792,7 @@ int __irq_set_trigger(struct irq_desc *desc, unsigned long flags)
case IRQ_SET_MASK_OK_DONE:
irqd_clear(&desc->irq_data, IRQD_TRIGGER_MASK);
irqd_set(&desc->irq_data, flags);
- /* fall through */
-
+ fallthrough;
case IRQ_SET_MASK_OK_NOCOPY:
flags = irqd_get_trigger_type(&desc->irq_data);
irq_settings_set_trigger_mask(desc, flags);
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 136ce049c4ad..05ce8a4d4729 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -651,11 +651,11 @@ int kallsyms_show_value(void)
case 0:
if (kallsyms_for_perf())
return 1;
- /* fallthrough */
+ fallthrough;
case 1:
if (has_capability_noaudit(current, CAP_SYSLOG))
return 1;
- /* fallthrough */
+ fallthrough;
default:
return 0;
}
diff --git a/kernel/pid.c b/kernel/pid.c
index 0a9f2e437217..b2a005a6dea1 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -144,7 +144,7 @@ void free_pid(struct pid *pid)
/* Handle a fork failure of the first process */
WARN_ON(ns->child_reaper);
ns->pid_allocated = 0;
- /* fall through */
+ fallthrough;
case 0:
schedule_work(&ns->proc_work);
break;
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 3c0a5a8170b0..d091dcd57557 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -647,7 +647,7 @@ static void power_down(void)
break;
case HIBERNATION_PLATFORM:
hibernation_platform_enter();
- /* Fall through */
+ fallthrough;
case HIBERNATION_SHUTDOWN:
if (pm_power_off)
kernel_power_off();
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 9568a2fe7c11..6bf5295b2ade 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -236,7 +236,7 @@ int pm_qos_update_target(struct pm_qos_constraints *c, struct plist_node *node,
* changed
*/
plist_del(node, &c->list);
- /* fall through */
+ fallthrough;
case PM_QOS_ADD_REQ:
plist_node_init(node, new_value);
plist_add(node, &c->list);
@@ -309,7 +309,7 @@ bool pm_qos_update_flags(struct pm_qos_flags *pqf,
break;
case PM_QOS_UPDATE_REQ:
pm_qos_flags_remove_req(pqf, req);
- /* fall through */
+ fallthrough;
case PM_QOS_ADD_REQ:
req->flags = val;
INIT_LIST_HEAD(&req->node);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ca65327a6de8..6b3d7c68e6fe 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1531,7 +1531,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
/* Read/clear last kernel messages */
case SYSLOG_ACTION_READ_CLEAR:
clear = true;
- /* FALL THRU */
+ fallthrough;
/* Read last kernel messages */
case SYSLOG_ACTION_READ_ALL:
if (!buf || len < 0)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index dd05a378631a..050b728728f4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2068,7 +2068,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
state = possible;
break;
}
- /* Fall-through */
+ fallthrough;
case possible:
do_set_cpus_allowed(p, cpu_possible_mask);
state = fail;
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index b5667a273bf6..7d6b84e0caca 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1224,13 +1224,13 @@ static void __free_domain_allocs(struct s_data *d, enum s_alloc what,
case sa_rootdomain:
if (!atomic_read(&d->rd->refcount))
free_rootdomain(&d->rd->rcu);
- /* Fall through */
+ fallthrough;
case sa_sd:
free_percpu(d->sd);
- /* Fall through */
+ fallthrough;
case sa_sd_storage:
__sdt_free(cpu_map);
- /* Fall through */
+ fallthrough;
case sa_none:
break;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index c4da1ef56fdf..73bdcc1f2561 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -846,7 +846,7 @@ static int check_kill_permission(int sig, struct kernel_siginfo *info,
*/
if (!sid || sid == task_session(current))
break;
- /* fall through */
+ fallthrough;
default:
return -EPERM;
}
diff --git a/kernel/sys.c b/kernel/sys.c
index a611d1d58c7d..bad4f30e7f37 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1737,8 +1737,7 @@ void getrusage(struct task_struct *p, int who, struct rusage *r)
if (who == RUSAGE_CHILDREN)
break;
- /* fall through */
-
+ fallthrough;
case RUSAGE_SELF:
thread_group_cputime_adjusted(p, &tgutime, &tgstime);
utime += tgutime;
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 0d4dc241c0fb..8060a35682e1 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -373,7 +373,7 @@ static bool hrtimer_fixup_activate(void *addr, enum debug_obj_state state)
switch (state) {
case ODEBUG_STATE_ACTIVE:
WARN_ON(1);
- /* fall through */
+ fallthrough;
default:
return false;
}
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 0ec5b7a1d769..6cc658391702 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -413,12 +413,12 @@ static struct pid *good_sigevent(sigevent_t * event)
rtn = pid_task(pid, PIDTYPE_PID);
if (!rtn || !same_thread_group(rtn, current))
return NULL;
- /* FALLTHRU */
+ fallthrough;
case SIGEV_SIGNAL:
case SIGEV_THREAD:
if (event->sigev_signo <= 0 || event->sigev_signo > SIGRTMAX)
return NULL;
- /* FALLTHRU */
+ fallthrough;
case SIGEV_NONE:
return pid;
default:
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index e51778c312f1..36d7464c8962 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -381,7 +381,7 @@ void tick_broadcast_control(enum tick_broadcast_mode mode)
switch (mode) {
case TICK_BROADCAST_FORCE:
tick_broadcast_forced = 1;
- /* fall through */
+ fallthrough;
case TICK_BROADCAST_ON:
cpumask_set_cpu(cpu, tick_broadcast_on);
if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 4820823515e9..6512d721ef57 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -653,7 +653,7 @@ static bool timer_fixup_activate(void *addr, enum debug_obj_state state)
case ODEBUG_STATE_ACTIVE:
WARN_ON(1);
- /* fall through */
+ fallthrough;
default:
return false;
}
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 2d6e93ab0478..0a1753dc69d3 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -717,7 +717,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
#endif
case BLKTRACESTART:
start = 1;
- /* fall through */
+ fallthrough;
case BLKTRACESTOP:
ret = __blk_trace_startstop(q, start);
break;
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index c9a74f82b14a..78b0bfc4d72e 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -499,7 +499,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
ptr++;
break;
}
- /* fall through */
+ fallthrough;
default:
parse_error(pe, FILT_ERR_TOO_MANY_PREDS,
next - str);
@@ -1273,7 +1273,7 @@ static int parse_pred(const char *str, void *data,
switch (op) {
case OP_NE:
pred->not = 1;
- /* Fall through */
+ fallthrough;
case OP_GLOB:
case OP_EQ:
break;
^ permalink raw reply related
* Re: WARNING: refcount bug in find_key_to_update
From: David Howells @ 2019-10-18 16:38 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, syzbot, aou, Jarkko Sakkinen, James Morris James Morris,
keyrings, Linux Kernel Mailing List, linux-riscv, LSM List,
Palmer Dabbelt, Serge E. Hallyn, syzkaller-bugs
In-Reply-To: <CAHk-=wjFozfjV34_qy3_Z155uz_Z7qFVfE8h=_9ceGU-SVk9hA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> The backtrace looks simple enough, though:
>
> RIP: 0010:refcount_inc_checked+0x2b/0x30 lib/refcount.c:156
> __key_get include/linux/key.h:281 [inline]
> find_key_to_update+0x67/0x80 security/keys/keyring.c:1127
> key_create_or_update+0x4e5/0xb20 security/keys/key.c:905
> __do_sys_add_key security/keys/keyctl.c:132 [inline]
> __se_sys_add_key security/keys/keyctl.c:72 [inline]
> __x64_sys_add_key+0x219/0x3f0 security/keys/keyctl.c:72
> do_syscall_64+0xd0/0x540 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> which to me implies that there's some locking bug, and somebody
> released the key without holding a lock.
>
> That code looks a bit confused to me. Releasing a key without holding
> a lock looks permitted, but if that's the case then __key_get() is
> complete garbage. It would need to use 'refcount_inc_not_zero()' and
> failure would require failing the caller.
find_key_to_update() must be called with the keyring-to-be-searched locked, as
stated in the comment on that function.
If a key-to-be-updated can be found in that keyring, then the keyring must be
holding a ref on that key already, so it's refcount must be > 0, so it
shouldn't be necessary to use refcount_inc_not_zero().
There shouldn't be a race with key_link(), key_unlink(), key_move(),
keyring_clear() or keyring_gc() (garbage collection) as all of those take a
write-lock on the keyring.
> But I haven't followed the key locking rules, so who knows. That "put
> without lock" scenario would explain the crash, though.
That shouldn't explain it. When key_put() reduces the refcount to 0, it just
schedules the garbage collector. It doesn't touch the key again directly.
I would guess that something incorrectly put a ref when it shouldn't have. Do
we know which type of key is involved? Looking at the syzkaller reproducer,
it's adding an encrypted key and a user key to the process keyring -
presumably repeating the procedure within the same process, hence how it finds
something to update.
David
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Theodore Y. Ts'o @ 2019-10-18 16:25 UTC (permalink / raw)
To: Brendan Higgins
Cc: shuah, john.johansen, jmorris, serge, keescook, alan.maguire,
yzaikin, davidgow, mcgrof, linux-kernel, linux-security-module,
kunit-dev, linux-kselftest, Mike Salvatore
In-Reply-To: <20191018004307.GA95597@google.com>
On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
> > +config SECURITY_APPARMOR_TEST
> > + bool "Build KUnit tests for policy_unpack.c"
> > + default n
> > + depends on KUNIT && SECURITY_APPARMOR
>
> Ted, here is an example where doing select on direct dependencies is
> tricky because SECURITY_APPARMOR has a number of indirect dependencies.
Well, that could be solved by adding a select on all of the indirect
dependencies. I did get your point about the fact that we could have
cases where the indirect dependencies might conflict with one another.
That's going to be a tough situation regardless of whether we have a
sat-solver or a human who has to struggle with that situation.
It's also going to be a bit sad because it means that we won't be able
to create a single config that could be used to run all the kunit
tests when a user pushes a change to a Gerrit server for review. :-/
I suppose that if we use a strict definition of "unit tests", and we
assume that all of the tests impacted by a change in foo/bar/baz.c
will be found in foo/bar/baz-test.c, or maybe foo/bar/*-test.c, we can
automate the generation of the kunitconfig file, perhaps?
The other sad bit about having mutually exclusive config options is
that we can't easily "run all KUinit tests" for some kind of test
spinner or zero-day bot.
I'm not sure there's a good solution to that issue, though.
- Ted
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Luis Chamberlain @ 2019-10-18 12:29 UTC (permalink / raw)
To: Brendan Higgins, Matthias Maennich
Cc: shuah, john.johansen, jmorris, serge, keescook, alan.maguire,
yzaikin, davidgow, tytso, linux-kernel, linux-security-module,
kunit-dev, linux-kselftest, Mike Salvatore
In-Reply-To: <20191018001816.94460-1-brendanhiggins@google.com>
On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> From: Mike Salvatore <mike.salvatore@canonical.com>
>
> 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.
And you'd run into the same situation expressed elsewhere with kunit of
an issue of the kunit test as built-in working but if built as a module
then it would not work, given the lack of exports. Symbols namespaces
should resolve this [0], and we'd be careful where a driver imports this
namespace.
[0] https://lwn.net/Articles/798254/
Luis
^ permalink raw reply
* Re: WARNING: refcount bug in find_key_to_update
From: Tetsuo Handa @ 2019-10-18 10:54 UTC (permalink / raw)
To: Eric Biggers; +Cc: Linus Torvalds, syzbot, keyrings, LSM List, syzkaller-bugs
In-Reply-To: <20191017160028.GA726@sol.localdomain>
On 2019/10/18 1:00, Eric Biggers wrote:
> The key is supposed to have refcount >= 1 since it's in a keyring.
> So some bug is causing it to have refcount 0. Perhaps some place calling
> key_put() too many times.
>
> Unfortunately I can't get the reproducer to work locally.
>
> Note that there are 2 other syzbot reports that look related.
> No reproducers for them, though:
>
> Title: KASAN: use-after-free Read in key_put
> Last occurred: 1 day ago
> Reported: 28 days ago
> Branches: Mainline
> Dashboard link: https://syzkaller.appspot.com/bug?id=f13750b1124e01191250cf930086dcc40740fa30
> Original thread: https://lore.kernel.org/lkml/0000000000008c3e590592cf4b7f@google.com/T/#u
>
> Title: KASAN: use-after-free Read in keyring_compare_object
> Last occurred: 49 days ago
> Reported: 84 days ago
> Branches: Mainline
> Dashboard link: https://syzkaller.appspot.com/bug?id=529ab6a98286c2a97c445988a62760a58d4a1d4b
> Original thread: https://lore.kernel.org/lkml/000000000000038ef6058e6f3592@google.com/T/#u
>
I don't know about keys, but I rather suspect lack of serialization locks between
"looking up for checking existing keys" versus "looking up for garbage collection".
Can we dump locks held by current thread when panic() is called?
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-10-18 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
In-Reply-To: <20191018001816.94460-1-brendanhiggins@google.com>
On Thu, Oct 17, 2019 at 05:18:16PM -0700, 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>
> ---
> Sorry in advanced for emailing so many people. I suspect that there are
> quite a few people who will be interested in this discussion - not so
> much because of the AppArmor test itself - but because I think this
> patch offers a good place to discuss how we should/will need to handle
> code visibility for the purpose of writing tests.
>
> I fully expect there to be a great deal of discussion on this patch
> especially on the topic of visibility and how to expose symbols for
> testing. Based on conversations I had before sending this patch out, I
> expect to see a number of different suggestions on how to test symbols
> that are not normally exposed.
> ---
> security/apparmor/Kconfig | 17 +
> security/apparmor/Makefile | 1 +
> security/apparmor/include/policy_unpack.h | 53 ++
> security/apparmor/policy_unpack.c | 61 +--
> security/apparmor/policy_unpack_test.c | 607 ++++++++++++++++++++++
> 5 files changed, 687 insertions(+), 52 deletions(-)
> create mode 100644 security/apparmor/policy_unpack_test.c
>
> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> index d8b1a360a636..632fbb873389 100644
> --- a/security/apparmor/Kconfig
> +++ b/security/apparmor/Kconfig
> @@ -66,3 +66,20 @@ 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_TEST
> + bool "Build KUnit tests for policy_unpack.c"
> + default n
> + depends on KUNIT && SECURITY_APPARMOR
Ted, here is an example where doing select on direct dependencies is
tricky because SECURITY_APPARMOR has a number of indirect dependencies.
> + 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/Makefile b/security/apparmor/Makefile
> index ff23fcfefe19..7ac1fc2dc045 100644
> --- a/security/apparmor/Makefile
> +++ b/security/apparmor/Makefile
> @@ -2,6 +2,7 @@
> # Makefile for AppArmor Linux Security Module
> #
> obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o
> +obj-$(CONFIG_SECURITY_APPARMOR_TEST) += policy_unpack_test.o
>
> apparmor-y := apparmorfs.o audit.o capability.o task.o ipc.o lib.o match.o \
> path.o domain.o policy.o policy_unpack.o procattr.o lsm.o \
> diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h
> index 46aefae918f5..dc272bafee2b 100644
> --- a/security/apparmor/include/policy_unpack.h
> +++ b/security/apparmor/include/policy_unpack.h
> @@ -16,6 +16,59 @@
> #include <linux/dcache.h>
> #include <linux/workqueue.h>
>
> +/*
> + * The AppArmor interface treats data as a type byte followed by the
> + * actual data. The interface has the notion of a a named entry
> + * which has a name (AA_NAME typecode followed by name string) followed by
> + * the entries typecode and data. Named types allow for optional
> + * elements and extensions to be added and tested for without breaking
> + * backwards compatibility.
> + */
> +
> +enum aa_code {
> + AA_U8,
> + AA_U16,
> + AA_U32,
> + AA_U64,
> + AA_NAME, /* same as string except it is items name */
> + AA_STRING,
> + AA_BLOB,
> + AA_STRUCT,
> + AA_STRUCTEND,
> + AA_LIST,
> + AA_LISTEND,
> + AA_ARRAY,
> + AA_ARRAYEND,
> +};
> +
> +/*
> + * aa_ext is the read of the buffer containing the serialized profile. The
> + * data is copied into a kernel buffer in apparmorfs and then handed off to
> + * the unpack routines.
> + */
> +struct aa_ext {
> + void *start;
> + void *end;
> + void *pos; /* pointer to current position in the buffer */
> + u32 version;
> +};
> +
> +/* test if read will be in packed data bounds */
> +static inline bool inbounds(struct aa_ext *e, size_t size)
> +{
> + return (size <= e->end - e->pos);
> +}
> +
> +size_t unpack_u16_chunk(struct aa_ext *e, char **chunk);
> +bool unpack_X(struct aa_ext *e, enum aa_code code);
> +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name);
> +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name);
> +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name);
> +size_t unpack_array(struct aa_ext *e, const char *name);
> +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name);
> +int unpack_str(struct aa_ext *e, const char **string, const char *name);
> +int unpack_strdup(struct aa_ext *e, char **string, const char *name);
> +
> struct aa_load_ent {
> struct list_head list;
> struct aa_profile *new;
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 8cfc9493eefc..7811e9b7a154 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -36,43 +36,6 @@
> #define v7 7
> #define v8 8 /* full network masking */
>
> -/*
> - * The AppArmor interface treats data as a type byte followed by the
> - * actual data. The interface has the notion of a a named entry
> - * which has a name (AA_NAME typecode followed by name string) followed by
> - * the entries typecode and data. Named types allow for optional
> - * elements and extensions to be added and tested for without breaking
> - * backwards compatibility.
> - */
> -
> -enum aa_code {
> - AA_U8,
> - AA_U16,
> - AA_U32,
> - AA_U64,
> - AA_NAME, /* same as string except it is items name */
> - AA_STRING,
> - AA_BLOB,
> - AA_STRUCT,
> - AA_STRUCTEND,
> - AA_LIST,
> - AA_LISTEND,
> - AA_ARRAY,
> - AA_ARRAYEND,
> -};
> -
> -/*
> - * aa_ext is the read of the buffer containing the serialized profile. The
> - * data is copied into a kernel buffer in apparmorfs and then handed off to
> - * the unpack routines.
> - */
> -struct aa_ext {
> - void *start;
> - void *end;
> - void *pos; /* pointer to current position in the buffer */
> - u32 version;
> -};
> -
> /* audit callback for unpack fields */
> static void audit_cb(struct audit_buffer *ab, void *va)
> {
> @@ -194,12 +157,6 @@ struct aa_loaddata *aa_loaddata_alloc(size_t size)
> return d;
> }
>
> -/* test if read will be in packed data bounds */
> -static bool inbounds(struct aa_ext *e, size_t size)
> -{
> - return (size <= e->end - e->pos);
> -}
> -
> static void *kvmemdup(const void *src, size_t len)
> {
> void *p = kvmalloc(len, GFP_KERNEL);
> @@ -216,7 +173,7 @@ static void *kvmemdup(const void *src, size_t len)
> *
> * Returns: the size of chunk found with the read head at the end of the chunk.
> */
> -static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
> +size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
Here and below are a number of parsing functions that we would like to
test.
I know that if we decide to just make them not-static, as I have done,
we will also want to properly namespace them. Nevertheless, I didn't
bother doing that yet since I suspect people may not actually want to
make them non-static.
In Mike's original version of this test[1], he just put the test in the
policy_unpack.c file thereby sidestepping the visibility issue entirely.
However, I am not a huge fan of mixing code and tests.
Another alternative, is just testing through the aa_unpack()[2] function
which is already visible; this has the advantage of not changing the
code under test and follows the principle of only testing public
interfaces, but has the downside that tests become more complicated as
each one has to produce a complete policy object.
I added Alan to this discussion because he has an idea for a function
called kunit_find_symbol()[3] which allows symbols to be looked up by name
so that they can be used.
Another possibility is to make a macro that makes the symbol only
visible when building with CONFIG_KUNIT enabled. It might look something
like this:
#if IS_ENABLED(CONFIG_KUNIT)
/**
* __visible_for_testing - Makes a static function visible when testing.
*
* A macro that replaces the `static` specifier on functions and global
* variables that is static when compiled normally and visible when compiled for
* tests.
*/
#define __visible_for_testing
#else
#define __visible_for_testing static
#endif
and would be used like this:
Instead of
static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
{...}
we would have
__visible_for_testing size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
{...}
in this case we would still probably want to properly namespace the
symbol to avoid the possibility of collisions when testing.
There were some other ideas that were mentioned; however, I will leave
it to those people to present their ideas.
> {
> size_t size = 0;
> void *pos = e->pos;
> @@ -237,7 +194,7 @@ static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
> }
>
> /* unpack control byte */
> -static bool unpack_X(struct aa_ext *e, enum aa_code code)
> +bool unpack_X(struct aa_ext *e, enum aa_code code)
> {
> if (!inbounds(e, 1))
> return 0;
> @@ -263,7 +220,7 @@ static bool unpack_X(struct aa_ext *e, enum aa_code code)
> *
> * Returns: 0 if either match fails, the read head does not move
> */
> -static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
> +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
> {
> /*
> * May need to reset pos if name or type doesn't match
> @@ -311,7 +268,7 @@ static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
> return 0;
> }
>
> -static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
> +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
> {
> void *pos = e->pos;
>
> @@ -329,7 +286,7 @@ static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
> return 0;
> }
>
> -static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
> +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
> {
> void *pos = e->pos;
>
> @@ -347,7 +304,7 @@ static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
> return 0;
> }
>
> -static size_t unpack_array(struct aa_ext *e, const char *name)
> +size_t unpack_array(struct aa_ext *e, const char *name)
> {
> void *pos = e->pos;
>
> @@ -365,7 +322,7 @@ static size_t unpack_array(struct aa_ext *e, const char *name)
> return 0;
> }
>
> -static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
> +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
> {
> void *pos = e->pos;
>
> @@ -387,7 +344,7 @@ static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
> return 0;
> }
>
> -static int unpack_str(struct aa_ext *e, const char **string, const char *name)
> +int unpack_str(struct aa_ext *e, const char **string, const char *name)
> {
> char *src_str;
> size_t size = 0;
> @@ -410,7 +367,7 @@ static int unpack_str(struct aa_ext *e, const char **string, const char *name)
> return 0;
> }
>
> -static int unpack_strdup(struct aa_ext *e, char **string, const char *name)
> +int unpack_strdup(struct aa_ext *e, char **string, const char *name)
> {
> const char *tmp;
> void *pos = e->pos;
> diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
> new file mode 100644
> index 000000000000..3907f7d642e6
> --- /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_EXPECT_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_EXPECT_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.23.0.866.gb869b98d4c-goog
[1] https://gitlab.com/apparmor/apparmor-kernel/blob/apparmor-kunit/security/apparmor/policy_unpack.c#L1066
[2] https://kunit-review.googlesource.com/c/linux/+/2809/3/security/apparmor/policy_unpack.c#b1054
[3] https://lore.kernel.org/linux-kselftest/alpine.LRH.2.20.1910111105350.21459@dhcp-10-175-191-48.vpn.oracle.com/
^ permalink raw reply
* Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Iurii Zaikin @ 2019-10-18 0:33 UTC (permalink / raw)
To: Brendan Higgins
Cc: shuah, john.johansen, jmorris, serge, keescook, alan.maguire,
davidgow, Luis Chamberlain, Theodore Ts'o,
Linux Kernel Mailing List, linux-security-module,
KUnit Development, open list:KERNEL SELFTEST FRAMEWORK,
Mike Salvatore
In-Reply-To: <20191018001816.94460-1-brendanhiggins@google.com>
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
> + depends on KUNIT && SECURITY_APPARMOR
> + help
>
select SECURITY_APPARMOR ?
> + 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
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));
^ permalink raw reply
* [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack
From: Brendan Higgins @ 2019-10-18 0:18 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>
---
Sorry in advanced for emailing so many people. I suspect that there are
quite a few people who will be interested in this discussion - not so
much because of the AppArmor test itself - but because I think this
patch offers a good place to discuss how we should/will need to handle
code visibility for the purpose of writing tests.
I fully expect there to be a great deal of discussion on this patch
especially on the topic of visibility and how to expose symbols for
testing. Based on conversations I had before sending this patch out, I
expect to see a number of different suggestions on how to test symbols
that are not normally exposed.
---
security/apparmor/Kconfig | 17 +
security/apparmor/Makefile | 1 +
security/apparmor/include/policy_unpack.h | 53 ++
security/apparmor/policy_unpack.c | 61 +--
security/apparmor/policy_unpack_test.c | 607 ++++++++++++++++++++++
5 files changed, 687 insertions(+), 52 deletions(-)
create mode 100644 security/apparmor/policy_unpack_test.c
diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
index d8b1a360a636..632fbb873389 100644
--- a/security/apparmor/Kconfig
+++ b/security/apparmor/Kconfig
@@ -66,3 +66,20 @@ 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_TEST
+ bool "Build KUnit tests for policy_unpack.c"
+ default n
+ 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/Makefile b/security/apparmor/Makefile
index ff23fcfefe19..7ac1fc2dc045 100644
--- a/security/apparmor/Makefile
+++ b/security/apparmor/Makefile
@@ -2,6 +2,7 @@
# Makefile for AppArmor Linux Security Module
#
obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o
+obj-$(CONFIG_SECURITY_APPARMOR_TEST) += policy_unpack_test.o
apparmor-y := apparmorfs.o audit.o capability.o task.o ipc.o lib.o match.o \
path.o domain.o policy.o policy_unpack.o procattr.o lsm.o \
diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h
index 46aefae918f5..dc272bafee2b 100644
--- a/security/apparmor/include/policy_unpack.h
+++ b/security/apparmor/include/policy_unpack.h
@@ -16,6 +16,59 @@
#include <linux/dcache.h>
#include <linux/workqueue.h>
+/*
+ * The AppArmor interface treats data as a type byte followed by the
+ * actual data. The interface has the notion of a a named entry
+ * which has a name (AA_NAME typecode followed by name string) followed by
+ * the entries typecode and data. Named types allow for optional
+ * elements and extensions to be added and tested for without breaking
+ * backwards compatibility.
+ */
+
+enum aa_code {
+ AA_U8,
+ AA_U16,
+ AA_U32,
+ AA_U64,
+ AA_NAME, /* same as string except it is items name */
+ AA_STRING,
+ AA_BLOB,
+ AA_STRUCT,
+ AA_STRUCTEND,
+ AA_LIST,
+ AA_LISTEND,
+ AA_ARRAY,
+ AA_ARRAYEND,
+};
+
+/*
+ * aa_ext is the read of the buffer containing the serialized profile. The
+ * data is copied into a kernel buffer in apparmorfs and then handed off to
+ * the unpack routines.
+ */
+struct aa_ext {
+ void *start;
+ void *end;
+ void *pos; /* pointer to current position in the buffer */
+ u32 version;
+};
+
+/* test if read will be in packed data bounds */
+static inline bool inbounds(struct aa_ext *e, size_t size)
+{
+ return (size <= e->end - e->pos);
+}
+
+size_t unpack_u16_chunk(struct aa_ext *e, char **chunk);
+bool unpack_X(struct aa_ext *e, enum aa_code code);
+bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name);
+bool unpack_u32(struct aa_ext *e, u32 *data, const char *name);
+bool unpack_u64(struct aa_ext *e, u64 *data, const char *name);
+size_t unpack_array(struct aa_ext *e, const char *name);
+size_t unpack_blob(struct aa_ext *e, char **blob, const char *name);
+int unpack_str(struct aa_ext *e, const char **string, const char *name);
+int unpack_strdup(struct aa_ext *e, char **string, const char *name);
+
struct aa_load_ent {
struct list_head list;
struct aa_profile *new;
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 8cfc9493eefc..7811e9b7a154 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -36,43 +36,6 @@
#define v7 7
#define v8 8 /* full network masking */
-/*
- * The AppArmor interface treats data as a type byte followed by the
- * actual data. The interface has the notion of a a named entry
- * which has a name (AA_NAME typecode followed by name string) followed by
- * the entries typecode and data. Named types allow for optional
- * elements and extensions to be added and tested for without breaking
- * backwards compatibility.
- */
-
-enum aa_code {
- AA_U8,
- AA_U16,
- AA_U32,
- AA_U64,
- AA_NAME, /* same as string except it is items name */
- AA_STRING,
- AA_BLOB,
- AA_STRUCT,
- AA_STRUCTEND,
- AA_LIST,
- AA_LISTEND,
- AA_ARRAY,
- AA_ARRAYEND,
-};
-
-/*
- * aa_ext is the read of the buffer containing the serialized profile. The
- * data is copied into a kernel buffer in apparmorfs and then handed off to
- * the unpack routines.
- */
-struct aa_ext {
- void *start;
- void *end;
- void *pos; /* pointer to current position in the buffer */
- u32 version;
-};
-
/* audit callback for unpack fields */
static void audit_cb(struct audit_buffer *ab, void *va)
{
@@ -194,12 +157,6 @@ struct aa_loaddata *aa_loaddata_alloc(size_t size)
return d;
}
-/* test if read will be in packed data bounds */
-static bool inbounds(struct aa_ext *e, size_t size)
-{
- return (size <= e->end - e->pos);
-}
-
static void *kvmemdup(const void *src, size_t len)
{
void *p = kvmalloc(len, GFP_KERNEL);
@@ -216,7 +173,7 @@ static void *kvmemdup(const void *src, size_t len)
*
* Returns: the size of chunk found with the read head at the end of the chunk.
*/
-static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
+size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
{
size_t size = 0;
void *pos = e->pos;
@@ -237,7 +194,7 @@ static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
}
/* unpack control byte */
-static bool unpack_X(struct aa_ext *e, enum aa_code code)
+bool unpack_X(struct aa_ext *e, enum aa_code code)
{
if (!inbounds(e, 1))
return 0;
@@ -263,7 +220,7 @@ static bool unpack_X(struct aa_ext *e, enum aa_code code)
*
* Returns: 0 if either match fails, the read head does not move
*/
-static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
+bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
{
/*
* May need to reset pos if name or type doesn't match
@@ -311,7 +268,7 @@ static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
return 0;
}
-static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
+bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
{
void *pos = e->pos;
@@ -329,7 +286,7 @@ static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
return 0;
}
-static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
+bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
{
void *pos = e->pos;
@@ -347,7 +304,7 @@ static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
return 0;
}
-static size_t unpack_array(struct aa_ext *e, const char *name)
+size_t unpack_array(struct aa_ext *e, const char *name)
{
void *pos = e->pos;
@@ -365,7 +322,7 @@ static size_t unpack_array(struct aa_ext *e, const char *name)
return 0;
}
-static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
+size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
{
void *pos = e->pos;
@@ -387,7 +344,7 @@ static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
return 0;
}
-static int unpack_str(struct aa_ext *e, const char **string, const char *name)
+int unpack_str(struct aa_ext *e, const char **string, const char *name)
{
char *src_str;
size_t size = 0;
@@ -410,7 +367,7 @@ static int unpack_str(struct aa_ext *e, const char **string, const char *name)
return 0;
}
-static int unpack_strdup(struct aa_ext *e, char **string, const char *name)
+int unpack_strdup(struct aa_ext *e, char **string, const char *name)
{
const char *tmp;
void *pos = e->pos;
diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
new file mode 100644
index 000000000000..3907f7d642e6
--- /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_EXPECT_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_EXPECT_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.23.0.866.gb869b98d4c-goog
^ permalink raw reply related
* Re: WARNING: refcount bug in find_key_to_update
From: Eric Biggers @ 2019-10-17 16:00 UTC (permalink / raw)
To: Linus Torvalds
Cc: syzbot, aou, David Howells, Jarkko Sakkinen,
James Morris James Morris, keyrings, Linux Kernel Mailing List,
linux-riscv, LSM List, Palmer Dabbelt, Serge E. Hallyn,
syzkaller-bugs
In-Reply-To: <CAHk-=wjFozfjV34_qy3_Z155uz_Z7qFVfE8h=_9ceGU-SVk9hA@mail.gmail.com>
On Thu, Oct 17, 2019 at 08:53:06AM -0700, Linus Torvalds wrote:
> On Wed, Oct 16, 2019 at 7:42 PM syzbot
> <syzbot+6455648abc28dbdd1e7f@syzkaller.appspotmail.com> wrote:
> >
> > syzbot has bisected this bug to 0570bc8b7c9b ("Merge tag
> > 'riscv/for-v5.3-rc1' ...")
>
> Yeah, that looks unlikely. The only non-riscv changes are from
> documentation updates and moving a config variable around.
>
> Looks like the crash is quite unlikely, and only happens in one out of
> ten runs for the ones it has happened to.
>
> The backtrace looks simple enough, though:
>
> RIP: 0010:refcount_inc_checked+0x2b/0x30 lib/refcount.c:156
> __key_get include/linux/key.h:281 [inline]
> find_key_to_update+0x67/0x80 security/keys/keyring.c:1127
> key_create_or_update+0x4e5/0xb20 security/keys/key.c:905
> __do_sys_add_key security/keys/keyctl.c:132 [inline]
> __se_sys_add_key security/keys/keyctl.c:72 [inline]
> __x64_sys_add_key+0x219/0x3f0 security/keys/keyctl.c:72
> do_syscall_64+0xd0/0x540 arch/x86/entry/common.c:296
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> which to me implies that there's some locking bug, and somebody
> released the key without holding a lock.
>
> That code looks a bit confused to me. Releasing a key without holding
> a lock looks permitted, but if that's the case then __key_get() is
> complete garbage. It would need to use 'refcount_inc_not_zero()' and
> failure would require failing the caller.
>
> But I haven't followed the key locking rules, so who knows. That "put
> without lock" scenario would explain the crash, though.
>
> David?
>
Yes this is a bogus bisection.
The key is supposed to have refcount >= 1 since it's in a keyring.
So some bug is causing it to have refcount 0. Perhaps some place calling
key_put() too many times.
Unfortunately I can't get the reproducer to work locally.
Note that there are 2 other syzbot reports that look related.
No reproducers for them, though:
Title: KASAN: use-after-free Read in key_put
Last occurred: 1 day ago
Reported: 28 days ago
Branches: Mainline
Dashboard link: https://syzkaller.appspot.com/bug?id=f13750b1124e01191250cf930086dcc40740fa30
Original thread: https://lore.kernel.org/lkml/0000000000008c3e590592cf4b7f@google.com/T/#u
Title: KASAN: use-after-free Read in keyring_compare_object
Last occurred: 49 days ago
Reported: 84 days ago
Branches: Mainline
Dashboard link: https://syzkaller.appspot.com/bug?id=529ab6a98286c2a97c445988a62760a58d4a1d4b
Original thread: https://lore.kernel.org/lkml/000000000000038ef6058e6f3592@google.com/T/#u
- Eric
^ permalink raw reply
* Re: WARNING: refcount bug in find_key_to_update
From: Linus Torvalds @ 2019-10-17 15:53 UTC (permalink / raw)
To: syzbot
Cc: aou, David Howells, Jarkko Sakkinen, James Morris James Morris,
keyrings, Linux Kernel Mailing List, linux-riscv, LSM List,
Palmer Dabbelt, Serge E. Hallyn, syzkaller-bugs
In-Reply-To: <00000000000071e2fc05951229ad@google.com>
On Wed, Oct 16, 2019 at 7:42 PM syzbot
<syzbot+6455648abc28dbdd1e7f@syzkaller.appspotmail.com> wrote:
>
> syzbot has bisected this bug to 0570bc8b7c9b ("Merge tag
> 'riscv/for-v5.3-rc1' ...")
Yeah, that looks unlikely. The only non-riscv changes are from
documentation updates and moving a config variable around.
Looks like the crash is quite unlikely, and only happens in one out of
ten runs for the ones it has happened to.
The backtrace looks simple enough, though:
RIP: 0010:refcount_inc_checked+0x2b/0x30 lib/refcount.c:156
__key_get include/linux/key.h:281 [inline]
find_key_to_update+0x67/0x80 security/keys/keyring.c:1127
key_create_or_update+0x4e5/0xb20 security/keys/key.c:905
__do_sys_add_key security/keys/keyctl.c:132 [inline]
__se_sys_add_key security/keys/keyctl.c:72 [inline]
__x64_sys_add_key+0x219/0x3f0 security/keys/keyctl.c:72
do_syscall_64+0xd0/0x540 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
which to me implies that there's some locking bug, and somebody
released the key without holding a lock.
That code looks a bit confused to me. Releasing a key without holding
a lock looks permitted, but if that's the case then __key_get() is
complete garbage. It would need to use 'refcount_inc_not_zero()' and
failure would require failing the caller.
But I haven't followed the key locking rules, so who knows. That "put
without lock" scenario would explain the crash, though.
David?
Linus
^ permalink raw reply
* Re: [RFC PATCH 03/21] pipe: Use head and tail pointers for the ring, not cursor and length
From: David Howells @ 2019-10-17 10:53 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: dhowells, torvalds, Casey Schaufler, Stephen Smalley,
Greg Kroah-Hartman, nicolas.dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, linux-security-module,
linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <b8799179-d389-8005-4f6d-845febc3bb23@rasmusvillemoes.dk>
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > (6) The number of free slots in the ring is "(tail + pipe->ring_size) -
> > head".
>
> Seems an odd way of writing pipe->ring_size - (head - tail) ; i.e.
> obviously #free slots is #size minus #occupancy.
Perhaps so. The way I was looking at it is the window into which things can
be written is tail...tail+ring_size; the number of free slots is the distance
from head to the end of the window.
Anyway, I now have a helper that does it your way.
> > (7) The ring is full if "head >= (tail + pipe->ring_size)", which can also
> > be written as "head - tail >= pipe->ring_size".
> >
>
> No it cannot, it _must_ be written in the latter form.
Ah, you're right. I have a helper now for that too.
> head-tail == pipe_size or head-tail >= pipe_size
In general, I'd prefer ">=" just in case tail gets in front of head.
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > Also split pipe->buffers into pipe->ring_size (which indicates the size of
> > the ring) and pipe->max_usage (which restricts the amount of ring that
> > write() is allowed to fill). This allows for a pipe that is both writable
> > by the kernel notification facility and by userspace, allowing plenty of
> > ring space for notifications to be added whilst preventing userspace from
> > being able to use up too much buffer space.
>
> That seems like something that should be added in a separate patch -
> adding ->max_usage and switching appropriate users of ->ring_size over,
> so it's more clear where you're using one or the other.
Okay.
> > + ibuf = &pipe->bufs[tail];
>
> I don't see where tail gets masked between tail = pipe->tail;
Yeah - I missed that one.
> In any case, how about seeding head and tail with something like 1<<20 when
> creating the pipe so bugs like that are hit more quickly.
That's sounds like a good idea.
> > + while (tail < head) {
> > + count += pipe->bufs[tail & mask].len;
> > + tail++;
> > }
>
> This is broken if head has wrapped but tail has not. It has to be "while
> (head - tail)" or perhaps just "while (tail != head)" or something along
> those lines.
Yeah... It's just too easy to overlook this and use ordinary comparisons.
I've switched to "while (tail != head)".
> > + mask = pipe->ring_size - 1;
> > + head = pipe->head & mask;
> > + tail = pipe->tail & mask;
> > + n = pipe->head - pipe->tail;
>
> I think it's confusing to "premask" head and tail here. Can you either
> drop that (pipe_set_size should hardly be a hot path?), or perhaps call
> them something else to avoid a future reader seeing an unmasked
> bufs[head] and thinking that's a bug?
I've made it now do the masking right before doing the memcpy calls and used
different variable names for it:
if (n > 0) {
unsigned int h = head & mask;
unsigned int t = tail & mask;
if (h > t) {
memcpy(bufs, &pipe->bufs + t,
n * sizeof(struct pipe_buffer));
} else {
unsigned int tsize = pipe->ring_size - t;
if (h > 0)
memcpy(bufs + tsize, pipe->bufs,
h * sizeof(struct pipe_buffer));
memcpy(bufs, pipe->bufs + t,
tsize * sizeof(struct pipe_buffer));
}
> > - data_start(i, &idx, start);
> > - /* some of this one + all after this one */
> > - npages = ((i->pipe->curbuf - idx - 1) & (i->pipe->buffers - 1)) + 1;
> > - capacity = min(npages,maxpages) * PAGE_SIZE - *start;
> > + data_start(i, &i_head, start);
> > + p_tail = i->pipe->tail;
> > + /* Amount of free space: some of this one + all after this one */
> > + npages = (p_tail + i->pipe->ring_size) - i_head;
>
> Hm, it's not clear that this is equivalent to the old computation. Since
> it seems repeated in a few places, could it be factored to a little
> helper (before this patch) and the "some of this one + all after this
> one" comment perhaps expanded to explain what is going on?
Yeah... It's a bit weird, even before my changes.
However, looking at it again, it seems data_start() does the appropriate
calculations. If there's space in the current head buffer, it returns the
offset to that and the head of that buffer, otherwise it advances the head
pointer and sets the offset to 0.
So I think the comment may actually be retrospective - referring to the state
that data_start() has given us, rather than talking about the next bit of
code.
I also wonder if pipe_get_pages_alloc() is broken. It doesn't check to see
whether the buffer is full at the point it calls data_start(). However,
data_start() doesn't check either and, without this patch, will simply advance
and mask off the ring index - which may wrap.
The maths in the unpatched version is pretty icky and I'm not convinced it's
correct.
David
^ permalink raw reply
* Re: WARNING: refcount bug in find_key_to_update
From: syzbot @ 2019-10-17 2:42 UTC (permalink / raw)
To: aou, dhowells, jarkko.sakkinen, jmorris, keyrings, linux-kernel,
linux-riscv, linux-security-module, palmer, serge, syzkaller-bugs,
torvalds
In-Reply-To: <000000000000830fe50595115344@google.com>
syzbot has bisected this bug to:
commit 0570bc8b7c9b41deba6f61ac218922e7168ad648
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu Jul 18 19:26:59 2019 +0000
Merge tag 'riscv/for-v5.3-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11b6e2bb600000
start commit: bc88f85c kthread: make __kthread_queue_delayed_work static
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=13b6e2bb600000
console output: https://syzkaller.appspot.com/x/log.txt?x=15b6e2bb600000
kernel config: https://syzkaller.appspot.com/x/.config?x=e0ac4d9b35046343
dashboard link: https://syzkaller.appspot.com/bug?extid=6455648abc28dbdd1e7f
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11c8adab600000
Reported-by: syzbot+6455648abc28dbdd1e7f@syzkaller.appspotmail.com
Fixes: 0570bc8b7c9b ("Merge tag 'riscv/for-v5.3-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* [PATCH] apparmor: Fix use-after-free in aa_audit_rule_init
From: Navid Emamdoost @ 2019-10-17 1:46 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, John Johansen,
James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In the implementation of aa_audit_rule_init(), when aa_label_parse()
fails the allocated memory for rule is released using
aa_audit_rule_free(). But after this release the the return statement
tries to access the label field of the rule which results in
use-after-free. Before releaseing the rule, copy errNo and return it
after releasing rule.
Fixes: 52e8c38001d8 ("apparmor: Fix memory leak of rule on error exit path")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
security/apparmor/audit.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 5a98661a8b46..48c15fb0aafe 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -178,6 +178,7 @@ void aa_audit_rule_free(void *vrule)
int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
{
struct aa_audit_rule *rule;
+ int err;
switch (field) {
case AUDIT_SUBJ_ROLE:
@@ -197,8 +198,9 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
GFP_KERNEL, true, false);
if (IS_ERR(rule->label)) {
+ err = rule->label;
aa_audit_rule_free(rule);
- return PTR_ERR(rule->label);
+ return PTR_ERR(err);
}
*vrule = rule;
--
2.17.1
^ permalink raw reply related
* WARNING: refcount bug in find_key_to_update
From: syzbot @ 2019-10-17 1:42 UTC (permalink / raw)
To: dhowells, jarkko.sakkinen, jmorris, keyrings, linux-kernel,
linux-security-module, serge, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: bc88f85c kthread: make __kthread_queue_delayed_work static
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1730584b600000
kernel config: https://syzkaller.appspot.com/x/.config?x=e0ac4d9b35046343
dashboard link: https://syzkaller.appspot.com/bug?extid=6455648abc28dbdd1e7f
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11c8adab600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+6455648abc28dbdd1e7f@syzkaller.appspotmail.com
------------[ cut here ]------------
refcount_t: increment on 0; use-after-free.
WARNING: CPU: 1 PID: 9064 at lib/refcount.c:156 refcount_inc_checked
lib/refcount.c:156 [inline]
WARNING: CPU: 1 PID: 9064 at lib/refcount.c:156
refcount_inc_checked+0x61/0x70 lib/refcount.c:154
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 9064 Comm: syz-executor.5 Not tainted 5.4.0-rc3+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
panic+0x2e3/0x75c kernel/panic.c:221
__warn.cold+0x2f/0x35 kernel/panic.c:582
report_bug+0x289/0x300 lib/bug.c:195
fixup_bug arch/x86/kernel/traps.c:179 [inline]
fixup_bug arch/x86/kernel/traps.c:174 [inline]
do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028
RIP: 0010:refcount_inc_checked lib/refcount.c:156 [inline]
RIP: 0010:refcount_inc_checked+0x61/0x70 lib/refcount.c:154
Code: 1d 58 46 7e 06 31 ff 89 de e8 0b cb 2e fe 84 db 75 dd e8 c2 c9 2e fe
48 c7 c7 40 ad e6 87 c6 05 38 46 7e 06 01 e8 67 0c 00 fe <0f> 0b eb c1 90
90 90 90 90 90 90 90 90 90 90 55 48 89 e5 41 57 41
RSP: 0018:ffff888081447c68 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff815cb646 RDI: ffffed1010288f7f
RBP: ffff888081447c78 R08: ffff8880a231a080 R09: ffffed1015d26159
R10: ffffed1015d26158 R11: ffff8880ae930ac7 R12: ffff8880a4518940
R13: 0000000000000000 R14: ffff888081447e10 R15: ffff8880a4518c40
__key_get include/linux/key.h:281 [inline]
find_key_to_update+0x8b/0xc0 security/keys/keyring.c:1127
key_create_or_update+0x588/0xbe0 security/keys/key.c:905
__do_sys_add_key security/keys/keyctl.c:132 [inline]
__se_sys_add_key security/keys/keyctl.c:72 [inline]
__x64_sys_add_key+0x2bd/0x4f0 security/keys/keyctl.c:72
do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x459a59
Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f22e3171c78 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 0000000000459a59
RDX: 0000000020000440 RSI: 0000000020000000 RDI: 0000000020000040
RBP: 000000000075bf20 R08: fffffffffffffffe R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000246 R12: 00007f22e31726d4
R13: 00000000004bfab8 R14: 00000000004d1ad8 R15: 00000000ffffffff
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [RFC PATCH 02/21] Add a prelocked wake-up
From: Tim Chen @ 2019-10-16 17:02 UTC (permalink / raw)
To: Linus Torvalds, David Howells, Kan Liang
Cc: Casey Schaufler, Stephen Smalley, Greg Kroah-Hartman,
Nicolas Dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, LSM List, linux-fsdevel, Linux API,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=whiz1sHXu8SVZKEC2dup=r5JMrftPtEt6ff9Ea8dyH8yQ@mail.gmail.com>
On 10/15/19 3:14 PM, Linus Torvalds wrote:
> On Tue, Oct 15, 2019 at 2:48 PM David Howells <dhowells@redhat.com> wrote:
>>
>> Add a wakeup call for a case whereby the caller already has the waitqueue
>> spinlock held.
>
> That naming is crazy.
>
> We already have helper functions like this, and they are just called
> "wake_up_locked()".
>
> So the "prelocked" naming is just odd. Make it be
> wake_up_interruptible_sync_poll_locked().
>
> The helper function should likely be
>
> void __wake_up_locked_sync_key(struct wait_queue_head *wq_head,
> unsigned int mode, void *key)
> {
> __wake_up_common(wq_head, mode, 1, WF_SYNC, key, NULL);
> }
> EXPORT_SYMBOL_GPL(__wake_up_locked_sync_key);
>
> to match the other naming patterns there.
>
> [ Unrelated ]
>
> Looking at that mess of functions, I also wonder if we should try to
> just remove the bookmark code again. It was cute, and it was useful,
> but I think the problem with the page lock list may have been fixed by
> commit 9a1ea439b16b ("mm: put_and_wait_on_page_locked() while page is
> migrated") which avoids the retry condition with
> migrate_page_move_mapping().
>
> Tim/Kan? Do you have the problematic load still?
>
Unfortunately, we do not have ready access to that problematic load
which was run by a customer on 8 socket system. They were not
willing to give the workload to us, and have not responded to my
request to rerun their load with commit 9a1ea439b16b.
The commit greatly reduced migration failures with concurrent page faulting.
And successful migrations could have prevented the big
pile up of waiters faulting and waiting on the page, which was the
problem the bookmark code was trying to solve.
So I also tend to think that the problem should have been resolved.
But unfortunately I don't have a ready workload to confirm.
Tim
^ permalink raw reply
* Re: [RFC PATCH 02/21] Add a prelocked wake-up
From: Linus Torvalds @ 2019-10-16 15:31 UTC (permalink / raw)
To: David Howells
Cc: Tim Chen, Kan Liang, Casey Schaufler, Stephen Smalley,
Greg Kroah-Hartman, Nicolas Dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <6900.1571235985@warthog.procyon.org.uk>
On Wed, Oct 16, 2019 at 7:26 AM David Howells <dhowells@redhat.com> wrote:
>
> Btw, is there any point in __wake_up_sync_key() taking a nr_exclusive
> argument since it clears WF_SYNC if nr_exclusive != 1 and doesn't make sense
> to be >1 anyway.
Ack, looks sane to me.
We have _very_ few users of nr_exclusive. I wonder if it's even worth
having at all, but it's definitely not worth it here.
I'd love for nr_exclusive to go away and be replaced by WF_ALL
instead. Right now it looks like there is one SGI driver that uses it,
and the sbitmap code. That was all I could find.
Oh well. You removing one case is at last a small amount of progress.
Linus
^ permalink raw reply
* Re: [RFC PATCH 02/21] Add a prelocked wake-up
From: David Howells @ 2019-10-16 14:26 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, Tim Chen, Kan Liang, Casey Schaufler, Stephen Smalley,
Greg Kroah-Hartman, Nicolas Dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=whiz1sHXu8SVZKEC2dup=r5JMrftPtEt6ff9Ea8dyH8yQ@mail.gmail.com>
Btw, is there any point in __wake_up_sync_key() taking a nr_exclusive
argument since it clears WF_SYNC if nr_exclusive != 1 and doesn't make sense
to be >1 anyway.
David
---
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 3eb7cae8206c..bb7676d396cd 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -201,9 +201,9 @@ void __wake_up(struct wait_queue_head *wq_head, unsigned int mode, int nr, void
void __wake_up_locked_key(struct wait_queue_head *wq_head, unsigned int mode, void *key);
void __wake_up_locked_key_bookmark(struct wait_queue_head *wq_head,
unsigned int mode, void *key, wait_queue_entry_t *bookmark);
-void __wake_up_sync_key(struct wait_queue_head *wq_head, unsigned int mode, int nr, void *key);
+void __wake_up_sync_key(struct wait_queue_head *wq_head, unsigned int mode, void *key);
void __wake_up_locked(struct wait_queue_head *wq_head, unsigned int mode, int nr);
-void __wake_up_sync(struct wait_queue_head *wq_head, unsigned int mode, int nr);
+void __wake_up_sync(struct wait_queue_head *wq_head, unsigned int mode);
#define wake_up(x) __wake_up(x, TASK_NORMAL, 1, NULL)
#define wake_up_nr(x, nr) __wake_up(x, TASK_NORMAL, nr, NULL)
@@ -214,7 +214,7 @@ void __wake_up_sync(struct wait_queue_head *wq_head, unsigned int mode, int nr);
#define wake_up_interruptible(x) __wake_up(x, TASK_INTERRUPTIBLE, 1, NULL)
#define wake_up_interruptible_nr(x, nr) __wake_up(x, TASK_INTERRUPTIBLE, nr, NULL)
#define wake_up_interruptible_all(x) __wake_up(x, TASK_INTERRUPTIBLE, 0, NULL)
-#define wake_up_interruptible_sync(x) __wake_up_sync((x), TASK_INTERRUPTIBLE, 1)
+#define wake_up_interruptible_sync(x) __wake_up_sync((x), TASK_INTERRUPTIBLE)
/*
* Wakeup macros to be used to report events to the targets.
@@ -228,7 +228,7 @@ void __wake_up_sync(struct wait_queue_head *wq_head, unsigned int mode, int nr);
#define wake_up_interruptible_poll(x, m) \
__wake_up(x, TASK_INTERRUPTIBLE, 1, poll_to_key(m))
#define wake_up_interruptible_sync_poll(x, m) \
- __wake_up_sync_key((x), TASK_INTERRUPTIBLE, 1, poll_to_key(m))
+ __wake_up_sync_key((x), TASK_INTERRUPTIBLE, poll_to_key(m))
#define ___wait_cond_timeout(condition) \
({ \
diff --git a/kernel/exit.c b/kernel/exit.c
index a46a50d67002..a1ff25ef050e 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1435,7 +1435,7 @@ static int child_wait_callback(wait_queue_entry_t *wait, unsigned mode,
void __wake_up_parent(struct task_struct *p, struct task_struct *parent)
{
__wake_up_sync_key(&parent->signal->wait_chldexit,
- TASK_INTERRUPTIBLE, 1, p);
+ TASK_INTERRUPTIBLE, p);
}
static long do_wait(struct wait_opts *wo)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index c1e566a114ca..b4b52361dab7 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -169,7 +169,6 @@ EXPORT_SYMBOL_GPL(__wake_up_locked_key_bookmark);
* __wake_up_sync_key - wake up threads blocked on a waitqueue.
* @wq_head: the waitqueue
* @mode: which threads
- * @nr_exclusive: how many wake-one or wake-many threads to wake up
* @key: opaque value to be passed to wakeup targets
*
* The sync wakeup differs that the waker knows that it will schedule
@@ -183,26 +182,21 @@ EXPORT_SYMBOL_GPL(__wake_up_locked_key_bookmark);
* accessing the task state.
*/
void __wake_up_sync_key(struct wait_queue_head *wq_head, unsigned int mode,
- int nr_exclusive, void *key)
+ void *key)
{
- int wake_flags = 1; /* XXX WF_SYNC */
-
if (unlikely(!wq_head))
return;
- if (unlikely(nr_exclusive != 1))
- wake_flags = 0;
-
- __wake_up_common_lock(wq_head, mode, nr_exclusive, wake_flags, key);
+ __wake_up_common_lock(wq_head, mode, 1, WF_SYNC, key);
}
EXPORT_SYMBOL_GPL(__wake_up_sync_key);
/*
* __wake_up_sync - see __wake_up_sync_key()
*/
-void __wake_up_sync(struct wait_queue_head *wq_head, unsigned int mode, int nr_exclusive)
+void __wake_up_sync(struct wait_queue_head *wq_head, unsigned int mode)
{
- __wake_up_sync_key(wq_head, mode, nr_exclusive, NULL);
+ __wake_up_sync_key(wq_head, mode, NULL);
}
EXPORT_SYMBOL_GPL(__wake_up_sync); /* For internal use only */
^ permalink raw reply related
* Re: [PATCH v2] perf_event: Add support for LSM and SELinux checks
From: Peter Zijlstra @ 2019-10-16 8:10 UTC (permalink / raw)
To: Joel Fernandes
Cc: Stephen Smalley, linux-kernel, rostedt, primiano, rsavitski,
jeffv, kernel-team, James Morris, Alexei Starovoitov,
Arnaldo Carvalho de Melo, bpf, Daniel Borkmann, Ingo Molnar,
Jiri Olsa, Kees Cook, linux-security-module, Matthew Garrett,
Namhyung Kim, selinux, Song Liu,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Yonghong Song
In-Reply-To: <20191016003500.GC89937@google.com>
On Tue, Oct 15, 2019 at 08:35:00PM -0400, Joel Fernandes wrote:
> Peter, if you are Ok with it, could you squash the below diff into my
> original patch? But let me know if you want me to resend the whole patch
> again. Thanks.
Folded thanks!
I had assumed it was required such that selinux/apparmour/etc.. could
use these values from their policy.
If that is not required, then moving them private is indeed the right
thing.
^ permalink raw reply
* Re: [RFC PATCH 03/21] pipe: Use head and tail pointers for the ring, not cursor and length
From: Rasmus Villemoes @ 2019-10-16 7:46 UTC (permalink / raw)
To: David Howells, torvalds
Cc: Casey Schaufler, Stephen Smalley, Greg Kroah-Hartman,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, linux-security-module, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <157117609543.15019.17103851546424902507.stgit@warthog.procyon.org.uk>
On 15/10/2019 23.48, David Howells wrote:
> Convert pipes to use head and tail pointers for the buffer ring rather than
> pointer and length as the latter requires two atomic ops to update (or a
> combined op) whereas the former only requires one.
>
> (1) The head pointer is the point at which production occurs and points to
> the slot in which the next buffer will be placed. This is equivalent
> to pipe->curbuf + pipe->nrbufs.
>
> The head pointer belongs to the write-side.
>
> (2) The tail pointer is the point at which consumption occurs. It points
> to the next slot to be consumed. This is equivalent to pipe->curbuf.
>
> The tail pointer belongs to the read-side.
>
> (3) head and tail are allowed to run to UINT_MAX and wrap naturally. They
> are only masked off when the array is being accessed, e.g.:
>
> pipe->bufs[head & mask]
>
> This means that it is not necessary to have a dead slot in the ring as
> head == tail isn't ambiguous.
>
> (4) The ring is empty if "head == tail".
>
> (5) The occupancy of the ring is "head - tail".
>
> (6) The number of free slots in the ring is "(tail + pipe->ring_size) -
> head".
Seems an odd way of writing pipe->ring_size - (head - tail) ; i.e.
obviously #free slots is #size minus #occupancy.
> (7) The ring is full if "head >= (tail + pipe->ring_size)", which can also
> be written as "head - tail >= pipe->ring_size".
>
No it cannot, it _must_ be written in the latter form. Assuming
sizeof(int)==1 for simplicity, consider ring_size = 16, tail = 240.
Regardless whether head is 240, 241, ..., 255, 0, tail + ring_size wraps
to 0, so the former expression states the ring is full in all cases.
Better spell out somewhere that while head and tail are free-running, at
any point in time they satisfy the invariant head - tail <= pipe_size
(and also 0 <= head - tail, but that's a tautology for unsigned
ints...). Then it's a matter of taste if one wants to write "full" as
head-tail == pipe_size or head-tail >= pipe_size.
> Also split pipe->buffers into pipe->ring_size (which indicates the size of the
> ring) and pipe->max_usage (which restricts the amount of ring that write() is
> allowed to fill). This allows for a pipe that is both writable by the kernel
> notification facility and by userspace, allowing plenty of ring space for
> notifications to be added whilst preventing userspace from being able to use
> up too much buffer space.
That seems like something that should be added in a separate patch -
adding ->max_usage and switching appropriate users of ->ring_size over,
so it's more clear where you're using one or the other.
> @@ -1949,8 +1950,12 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
>
> pipe_lock(pipe);
>
> - bufs = kvmalloc_array(pipe->nrbufs, sizeof(struct pipe_buffer),
> - GFP_KERNEL);
> + head = pipe->head;
> + tail = pipe->tail;
> + mask = pipe->ring_size - 1;
> + count = head - tail;
> +
> + bufs = kvmalloc_array(count, sizeof(struct pipe_buffer), GFP_KERNEL);
> if (!bufs) {
> pipe_unlock(pipe);
> return -ENOMEM;
> @@ -1958,8 +1963,8 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
>
> nbuf = 0;
> rem = 0;
> - for (idx = 0; idx < pipe->nrbufs && rem < len; idx++)
> - rem += pipe->bufs[(pipe->curbuf + idx) & (pipe->buffers - 1)].len;
> + for (idx = tail; idx < head && rem < len; idx++)
> + rem += pipe->bufs[idx & mask].len;
>
> ret = -EINVAL;
> if (rem < len)
> @@ -1970,16 +1975,16 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> struct pipe_buffer *ibuf;
> struct pipe_buffer *obuf;
>
> - BUG_ON(nbuf >= pipe->buffers);
> - BUG_ON(!pipe->nrbufs);
> - ibuf = &pipe->bufs[pipe->curbuf];
> + BUG_ON(nbuf >= pipe->ring_size);
> + BUG_ON(tail == head);
> + ibuf = &pipe->bufs[tail];
I don't see where tail gets masked between tail = pipe->tail; above and
here, but I may be missing it. In any case, how about seeding head and
tail with something like 1<<20 when creating the pipe so bugs like that
are hit more quickly.
> @@ -515,17 +525,19 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
> static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> struct pipe_inode_info *pipe = filp->private_data;
> - int count, buf, nrbufs;
> + int count, head, tail, mask;
>
> switch (cmd) {
> case FIONREAD:
> __pipe_lock(pipe);
> count = 0;
> - buf = pipe->curbuf;
> - nrbufs = pipe->nrbufs;
> - while (--nrbufs >= 0) {
> - count += pipe->bufs[buf].len;
> - buf = (buf+1) & (pipe->buffers - 1);
> + head = pipe->head;
> + tail = pipe->tail;
> + mask = pipe->ring_size - 1;
> +
> + while (tail < head) {
> + count += pipe->bufs[tail & mask].len;
> + tail++;
> }
This is broken if head has wrapped but tail has not. It has to be "while
(head - tail)" or perhaps just "while (tail != head)" or something along
those lines.
> @@ -1086,17 +1104,21 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
> }
>
> /*
> - * We can shrink the pipe, if arg >= pipe->nrbufs. Since we don't
> - * expect a lot of shrink+grow operations, just free and allocate
> - * again like we would do for growing. If the pipe currently
> + * We can shrink the pipe, if arg is greater than the ring occupancy.
> + * Since we don't expect a lot of shrink+grow operations, just free and
> + * allocate again like we would do for growing. If the pipe currently
> * contains more buffers than arg, then return busy.
> */
> - if (nr_pages < pipe->nrbufs) {
> + mask = pipe->ring_size - 1;
> + head = pipe->head & mask;
> + tail = pipe->tail & mask;
> + n = pipe->head - pipe->tail;
I think it's confusing to "premask" head and tail here. Can you either
drop that (pipe_set_size should hardly be a hot path?), or perhaps call
them something else to avoid a future reader seeing an unmasked
bufs[head] and thinking that's a bug?
> @@ -1254,9 +1290,10 @@ static ssize_t pipe_get_pages(struct iov_iter *i,
> struct page **pages, size_t maxsize, unsigned maxpages,
> size_t *start)
> {
> + unsigned int p_tail;
> + unsigned int i_head;
> unsigned npages;
> size_t capacity;
> - int idx;
>
> if (!maxsize)
> return 0;
> @@ -1264,12 +1301,15 @@ static ssize_t pipe_get_pages(struct iov_iter *i,
> if (!sanity(i))
> return -EFAULT;
>
> - data_start(i, &idx, start);
> - /* some of this one + all after this one */
> - npages = ((i->pipe->curbuf - idx - 1) & (i->pipe->buffers - 1)) + 1;
> - capacity = min(npages,maxpages) * PAGE_SIZE - *start;
> + data_start(i, &i_head, start);
> + p_tail = i->pipe->tail;
> + /* Amount of free space: some of this one + all after this one */
> + npages = (p_tail + i->pipe->ring_size) - i_head;
Hm, it's not clear that this is equivalent to the old computation. Since
it seems repeated in a few places, could it be factored to a little
helper (before this patch) and the "some of this one + all after this
one" comment perhaps expanded to explain what is going on?
Rasmus
^ permalink raw reply
* [Patch v8 4/4] KEYS: trusted: Move TPM2 trusted keys code
From: Sumit Garg @ 2019-10-16 5:14 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1571202895-32651-1-git-send-email-sumit.garg@linaro.org>
Move TPM2 trusted keys code to trusted keys subsystem. The reason
being it's better to consolidate all the trusted keys code to a single
location so that it can be maintained sanely.
Also, utilize existing tpm_send() exported API which wraps the internal
tpm_transmit_cmd() API.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/char/tpm/tpm-interface.c | 56 ------
drivers/char/tpm/tpm.h | 11 --
drivers/char/tpm/tpm2-cmd.c | 307 -----------------------------
include/keys/trusted_tpm.h | 7 +
include/linux/tpm.h | 36 ++--
security/keys/trusted-keys/Makefile | 1 +
security/keys/trusted-keys/trusted_tpm1.c | 4 +-
security/keys/trusted-keys/trusted_tpm2.c | 314 ++++++++++++++++++++++++++++++
8 files changed, 342 insertions(+), 394 deletions(-)
create mode 100644 security/keys/trusted-keys/trusted_tpm2.c
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 7f10549..a438b12 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -459,62 +459,6 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
}
EXPORT_SYMBOL_GPL(tpm_get_random);
-/**
- * tpm_seal_trusted() - seal a trusted key payload
- * @chip: a &struct tpm_chip instance, %NULL for the default chip
- * @options: authentication values and other options
- * @payload: the key data in clear and encrypted form
- *
- * Note: only TPM 2.0 chip are supported. TPM 1.x implementation is located in
- * the keyring subsystem.
- *
- * Return: same as with tpm_transmit_cmd()
- */
-int tpm_seal_trusted(struct tpm_chip *chip, struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- int rc;
-
- chip = tpm_find_get_ops(chip);
- if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
- return -ENODEV;
-
- rc = tpm2_seal_trusted(chip, payload, options);
-
- tpm_put_ops(chip);
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_seal_trusted);
-
-/**
- * tpm_unseal_trusted() - unseal a trusted key
- * @chip: a &struct tpm_chip instance, %NULL for the default chip
- * @options: authentication values and other options
- * @payload: the key data in clear and encrypted form
- *
- * Note: only TPM 2.0 chip are supported. TPM 1.x implementation is located in
- * the keyring subsystem.
- *
- * Return: same as with tpm_transmit_cmd()
- */
-int tpm_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- int rc;
-
- chip = tpm_find_get_ops(chip);
- if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
- return -ENODEV;
-
- rc = tpm2_unseal_trusted(chip, payload, options);
-
- tpm_put_ops(chip);
-
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_unseal_trusted);
-
static int __init tpm_init(void)
{
int rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b174cf4..b9e1547 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -212,11 +212,6 @@ static inline void tpm_add_ppi(struct tpm_chip *chip)
}
#endif
-static inline u32 tpm2_rc_value(u32 rc)
-{
- return (rc & BIT(7)) ? rc & 0xff : rc;
-}
-
int tpm2_get_timeouts(struct tpm_chip *chip);
int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digest, u16 *digest_size_ptr);
@@ -224,12 +219,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests);
int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
-int tpm2_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
-int tpm2_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 5817dfe..fdb4577 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -13,20 +13,6 @@
#include "tpm.h"
#include <crypto/hash_info.h>
-#include <keys/trusted-type.h>
-
-enum tpm2_object_attributes {
- TPM2_OA_USER_WITH_AUTH = BIT(6),
-};
-
-enum tpm2_session_attributes {
- TPM2_SA_CONTINUE_SESSION = BIT(0),
-};
-
-struct tpm2_hash {
- unsigned int crypto_id;
- unsigned int tpm_id;
-};
static struct tpm2_hash tpm2_hash_map[] = {
{HASH_ALGO_SHA1, TPM_ALG_SHA1},
@@ -377,299 +363,6 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
tpm_buf_destroy(&buf);
}
-/**
- * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
- *
- * @buf: an allocated tpm_buf instance
- * @session_handle: session handle
- * @nonce: the session nonce, may be NULL if not used
- * @nonce_len: the session nonce length, may be 0 if not used
- * @attributes: the session attributes
- * @hmac: the session HMAC or password, may be NULL if not used
- * @hmac_len: the session HMAC or password length, maybe 0 if not used
- */
-static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
- const u8 *nonce, u16 nonce_len,
- u8 attributes,
- const u8 *hmac, u16 hmac_len)
-{
- tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
- tpm_buf_append_u32(buf, session_handle);
- tpm_buf_append_u16(buf, nonce_len);
-
- if (nonce && nonce_len)
- tpm_buf_append(buf, nonce, nonce_len);
-
- tpm_buf_append_u8(buf, attributes);
- tpm_buf_append_u16(buf, hmac_len);
-
- if (hmac && hmac_len)
- tpm_buf_append(buf, hmac, hmac_len);
-}
-
-/**
- * tpm2_seal_trusted() - seal the payload of a trusted key
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- *
- * Return: < 0 on error and 0 on success.
- */
-int tpm2_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- unsigned int blob_len;
- struct tpm_buf buf;
- u32 hash;
- int i;
- int rc;
-
- for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
- if (options->hash == tpm2_hash_map[i].crypto_id) {
- hash = tpm2_hash_map[i].tpm_id;
- break;
- }
- }
-
- if (i == ARRAY_SIZE(tpm2_hash_map))
- return -EINVAL;
-
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(&buf, options->keyhandle);
- tpm2_buf_append_auth(&buf, TPM2_RS_PW,
- NULL /* nonce */, 0,
- 0 /* session_attributes */,
- options->keyauth /* hmac */,
- TPM_DIGEST_SIZE);
-
- /* sensitive */
- tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
-
- tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
- tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
- tpm_buf_append_u16(&buf, payload->key_len + 1);
- tpm_buf_append(&buf, payload->key, payload->key_len);
- tpm_buf_append_u8(&buf, payload->migratable);
-
- /* public */
- tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
- tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
- tpm_buf_append_u16(&buf, hash);
-
- /* policy */
- if (options->policydigest_len) {
- tpm_buf_append_u32(&buf, 0);
- tpm_buf_append_u16(&buf, options->policydigest_len);
- tpm_buf_append(&buf, options->policydigest,
- options->policydigest_len);
- } else {
- tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
- tpm_buf_append_u16(&buf, 0);
- }
-
- /* public parameters */
- tpm_buf_append_u16(&buf, TPM_ALG_NULL);
- tpm_buf_append_u16(&buf, 0);
-
- /* outside info */
- tpm_buf_append_u16(&buf, 0);
-
- /* creation PCR */
- tpm_buf_append_u32(&buf, 0);
-
- if (buf.flags & TPM_BUF_OVERFLOW) {
- rc = -E2BIG;
- goto out;
- }
-
- rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
- if (rc)
- goto out;
-
- blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
- if (blob_len > MAX_BLOB_SIZE) {
- rc = -E2BIG;
- goto out;
- }
- if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
- rc = -EFAULT;
- goto out;
- }
-
- memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len);
- payload->blob_len = blob_len;
-
-out:
- tpm_buf_destroy(&buf);
-
- if (rc > 0) {
- if (tpm2_rc_value(rc) == TPM2_RC_HASH)
- rc = -EINVAL;
- else
- rc = -EPERM;
- }
-
- return rc;
-}
-
-/**
- * tpm2_load_cmd() - execute a TPM2_Load command
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- * @blob_handle: returned blob handle
- *
- * Return: 0 on success.
- * -E2BIG on wrong payload size.
- * -EPERM on tpm error status.
- * < 0 error from tpm_transmit_cmd.
- */
-static int tpm2_load_cmd(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options,
- u32 *blob_handle)
-{
- struct tpm_buf buf;
- unsigned int private_len;
- unsigned int public_len;
- unsigned int blob_len;
- int rc;
-
- private_len = be16_to_cpup((__be16 *) &payload->blob[0]);
- if (private_len > (payload->blob_len - 2))
- return -E2BIG;
-
- public_len = be16_to_cpup((__be16 *) &payload->blob[2 + private_len]);
- blob_len = private_len + public_len + 4;
- if (blob_len > payload->blob_len)
- return -E2BIG;
-
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(&buf, options->keyhandle);
- tpm2_buf_append_auth(&buf, TPM2_RS_PW,
- NULL /* nonce */, 0,
- 0 /* session_attributes */,
- options->keyauth /* hmac */,
- TPM_DIGEST_SIZE);
-
- tpm_buf_append(&buf, payload->blob, blob_len);
-
- if (buf.flags & TPM_BUF_OVERFLOW) {
- rc = -E2BIG;
- goto out;
- }
-
- rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
- if (!rc)
- *blob_handle = be32_to_cpup(
- (__be32 *) &buf.data[TPM_HEADER_SIZE]);
-
-out:
- tpm_buf_destroy(&buf);
-
- if (rc > 0)
- rc = -EPERM;
-
- return rc;
-}
-
-/**
- * tpm2_unseal_cmd() - execute a TPM2_Unload command
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- * @blob_handle: blob handle
- *
- * Return: 0 on success
- * -EPERM on tpm error status
- * < 0 error from tpm_transmit_cmd
- */
-static int tpm2_unseal_cmd(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options,
- u32 blob_handle)
-{
- struct tpm_buf buf;
- u16 data_len;
- u8 *data;
- int rc;
-
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(&buf, blob_handle);
- tpm2_buf_append_auth(&buf,
- options->policyhandle ?
- options->policyhandle : TPM2_RS_PW,
- NULL /* nonce */, 0,
- TPM2_SA_CONTINUE_SESSION,
- options->blobauth /* hmac */,
- TPM_DIGEST_SIZE);
-
- rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
- if (rc > 0)
- rc = -EPERM;
-
- if (!rc) {
- data_len = be16_to_cpup(
- (__be16 *) &buf.data[TPM_HEADER_SIZE + 4]);
- if (data_len < MIN_KEY_SIZE || data_len > MAX_KEY_SIZE + 1) {
- rc = -EFAULT;
- goto out;
- }
-
- if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 6 + data_len) {
- rc = -EFAULT;
- goto out;
- }
- data = &buf.data[TPM_HEADER_SIZE + 6];
-
- memcpy(payload->key, data, data_len - 1);
- payload->key_len = data_len - 1;
- payload->migratable = data[data_len - 1];
- }
-
-out:
- tpm_buf_destroy(&buf);
- return rc;
-}
-
-/**
- * tpm2_unseal_trusted() - unseal the payload of a trusted key
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- *
- * Return: Same as with tpm_transmit_cmd.
- */
-int tpm2_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- u32 blob_handle;
- int rc;
-
- rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
- if (rc)
- return rc;
-
- rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
- tpm2_flush_context(chip, blob_handle);
- return rc;
-}
-
struct tpm2_get_cap_out {
u8 more_data;
__be32 subcap_id;
diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h
index 7b9d7b4..a56d8e1 100644
--- a/include/keys/trusted_tpm.h
+++ b/include/keys/trusted_tpm.h
@@ -40,6 +40,13 @@ int TSS_checkhmac1(unsigned char *buffer,
int trusted_tpm_send(unsigned char *cmd, size_t buflen);
int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce);
+int tpm2_seal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options);
+int tpm2_unseal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options);
+
#define TPM_DEBUG 0
#if TPM_DEBUG
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index c78119f..0d6e949 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -296,6 +296,19 @@ struct tpm_buf {
u8 *data;
};
+enum tpm2_object_attributes {
+ TPM2_OA_USER_WITH_AUTH = BIT(6),
+};
+
+enum tpm2_session_attributes {
+ TPM2_SA_CONTINUE_SESSION = BIT(0),
+};
+
+struct tpm2_hash {
+ unsigned int crypto_id;
+ unsigned int tpm_id;
+};
+
static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
{
struct tpm_header *head = (struct tpm_header *)buf->data;
@@ -375,6 +388,11 @@ static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
tpm_buf_append(buf, (u8 *) &value2, 4);
}
+static inline u32 tpm2_rc_value(u32 rc)
+{
+ return (rc & BIT(7)) ? rc & 0xff : rc;
+}
+
#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
extern int tpm_is_tpm2(struct tpm_chip *chip);
@@ -384,12 +402,6 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests);
extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
-extern int tpm_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
-extern int tpm_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
extern struct tpm_chip *tpm_default_chip(void);
#else
static inline int tpm_is_tpm2(struct tpm_chip *chip)
@@ -418,18 +430,6 @@ static inline int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max)
return -ENODEV;
}
-static inline int tpm_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- return -ENODEV;
-}
-static inline int tpm_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- return -ENODEV;
-}
static inline struct tpm_chip *tpm_default_chip(void)
{
return NULL;
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index 1a24680..7b73ceb 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -5,3 +5,4 @@
obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
trusted-y += trusted_tpm1.o
+trusted-y += trusted_tpm2.o
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index e3155fd..eb5074e 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1004,7 +1004,7 @@ static int trusted_instantiate(struct key *key,
switch (key_cmd) {
case Opt_load:
if (tpm2)
- ret = tpm_unseal_trusted(chip, payload, options);
+ ret = tpm2_unseal_trusted(chip, payload, options);
else
ret = key_unseal(payload, options);
dump_payload(payload);
@@ -1020,7 +1020,7 @@ static int trusted_instantiate(struct key *key,
goto out;
}
if (tpm2)
- ret = tpm_seal_trusted(chip, payload, options);
+ ret = tpm2_seal_trusted(chip, payload, options);
else
ret = key_seal(payload, options);
if (ret < 0)
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
new file mode 100644
index 0000000..a9810ac
--- /dev/null
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2014 Intel Corporation
+ */
+
+#include <linux/string.h>
+#include <linux/err.h>
+#include <linux/tpm.h>
+#include <linux/tpm_command.h>
+
+#include <keys/trusted-type.h>
+#include <keys/trusted_tpm.h>
+
+static struct tpm2_hash tpm2_hash_map[] = {
+ {HASH_ALGO_SHA1, TPM_ALG_SHA1},
+ {HASH_ALGO_SHA256, TPM_ALG_SHA256},
+ {HASH_ALGO_SHA384, TPM_ALG_SHA384},
+ {HASH_ALGO_SHA512, TPM_ALG_SHA512},
+ {HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
+};
+
+/**
+ * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
+ *
+ * @buf: an allocated tpm_buf instance
+ * @session_handle: session handle
+ * @nonce: the session nonce, may be NULL if not used
+ * @nonce_len: the session nonce length, may be 0 if not used
+ * @attributes: the session attributes
+ * @hmac: the session HMAC or password, may be NULL if not used
+ * @hmac_len: the session HMAC or password length, maybe 0 if not used
+ */
+static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
+ const u8 *nonce, u16 nonce_len,
+ u8 attributes,
+ const u8 *hmac, u16 hmac_len)
+{
+ tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
+ tpm_buf_append_u32(buf, session_handle);
+ tpm_buf_append_u16(buf, nonce_len);
+
+ if (nonce && nonce_len)
+ tpm_buf_append(buf, nonce, nonce_len);
+
+ tpm_buf_append_u8(buf, attributes);
+ tpm_buf_append_u16(buf, hmac_len);
+
+ if (hmac && hmac_len)
+ tpm_buf_append(buf, hmac, hmac_len);
+}
+
+/**
+ * tpm2_seal_trusted() - seal the payload of a trusted key
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ *
+ * Return: < 0 on error and 0 on success.
+ */
+int tpm2_seal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options)
+{
+ unsigned int blob_len;
+ struct tpm_buf buf;
+ u32 hash;
+ int i;
+ int rc;
+
+ for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+ if (options->hash == tpm2_hash_map[i].crypto_id) {
+ hash = tpm2_hash_map[i].tpm_id;
+ break;
+ }
+ }
+
+ if (i == ARRAY_SIZE(tpm2_hash_map))
+ return -EINVAL;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(&buf, options->keyhandle);
+ tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+ NULL /* nonce */, 0,
+ 0 /* session_attributes */,
+ options->keyauth /* hmac */,
+ TPM_DIGEST_SIZE);
+
+ /* sensitive */
+ tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
+
+ tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
+ tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
+ tpm_buf_append_u16(&buf, payload->key_len + 1);
+ tpm_buf_append(&buf, payload->key, payload->key_len);
+ tpm_buf_append_u8(&buf, payload->migratable);
+
+ /* public */
+ tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
+ tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
+ tpm_buf_append_u16(&buf, hash);
+
+ /* policy */
+ if (options->policydigest_len) {
+ tpm_buf_append_u32(&buf, 0);
+ tpm_buf_append_u16(&buf, options->policydigest_len);
+ tpm_buf_append(&buf, options->policydigest,
+ options->policydigest_len);
+ } else {
+ tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
+ tpm_buf_append_u16(&buf, 0);
+ }
+
+ /* public parameters */
+ tpm_buf_append_u16(&buf, TPM_ALG_NULL);
+ tpm_buf_append_u16(&buf, 0);
+
+ /* outside info */
+ tpm_buf_append_u16(&buf, 0);
+
+ /* creation PCR */
+ tpm_buf_append_u32(&buf, 0);
+
+ if (buf.flags & TPM_BUF_OVERFLOW) {
+ rc = -E2BIG;
+ goto out;
+ }
+
+ rc = tpm_send(chip, buf.data, tpm_buf_length(&buf));
+ if (rc)
+ goto out;
+
+ blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
+ if (blob_len > MAX_BLOB_SIZE) {
+ rc = -E2BIG;
+ goto out;
+ }
+ if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
+ rc = -EFAULT;
+ goto out;
+ }
+
+ memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len);
+ payload->blob_len = blob_len;
+
+out:
+ tpm_buf_destroy(&buf);
+
+ if (rc > 0) {
+ if (tpm2_rc_value(rc) == TPM2_RC_HASH)
+ rc = -EINVAL;
+ else
+ rc = -EPERM;
+ }
+
+ return rc;
+}
+
+/**
+ * tpm2_load_cmd() - execute a TPM2_Load command
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ * @blob_handle: returned blob handle
+ *
+ * Return: 0 on success.
+ * -E2BIG on wrong payload size.
+ * -EPERM on tpm error status.
+ * < 0 error from tpm_send.
+ */
+static int tpm2_load_cmd(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options,
+ u32 *blob_handle)
+{
+ struct tpm_buf buf;
+ unsigned int private_len;
+ unsigned int public_len;
+ unsigned int blob_len;
+ int rc;
+
+ private_len = be16_to_cpup((__be16 *) &payload->blob[0]);
+ if (private_len > (payload->blob_len - 2))
+ return -E2BIG;
+
+ public_len = be16_to_cpup((__be16 *) &payload->blob[2 + private_len]);
+ blob_len = private_len + public_len + 4;
+ if (blob_len > payload->blob_len)
+ return -E2BIG;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(&buf, options->keyhandle);
+ tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+ NULL /* nonce */, 0,
+ 0 /* session_attributes */,
+ options->keyauth /* hmac */,
+ TPM_DIGEST_SIZE);
+
+ tpm_buf_append(&buf, payload->blob, blob_len);
+
+ if (buf.flags & TPM_BUF_OVERFLOW) {
+ rc = -E2BIG;
+ goto out;
+ }
+
+ rc = tpm_send(chip, buf.data, tpm_buf_length(&buf));
+ if (!rc)
+ *blob_handle = be32_to_cpup(
+ (__be32 *) &buf.data[TPM_HEADER_SIZE]);
+
+out:
+ tpm_buf_destroy(&buf);
+
+ if (rc > 0)
+ rc = -EPERM;
+
+ return rc;
+}
+
+/**
+ * tpm2_unseal_cmd() - execute a TPM2_Unload command
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ * @blob_handle: blob handle
+ *
+ * Return: 0 on success
+ * -EPERM on tpm error status
+ * < 0 error from tpm_send
+ */
+static int tpm2_unseal_cmd(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options,
+ u32 blob_handle)
+{
+ struct tpm_buf buf;
+ u16 data_len;
+ u8 *data;
+ int rc;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(&buf, blob_handle);
+ tpm2_buf_append_auth(&buf,
+ options->policyhandle ?
+ options->policyhandle : TPM2_RS_PW,
+ NULL /* nonce */, 0,
+ TPM2_SA_CONTINUE_SESSION,
+ options->blobauth /* hmac */,
+ TPM_DIGEST_SIZE);
+
+ rc = tpm_send(chip, buf.data, tpm_buf_length(&buf));
+ if (rc > 0)
+ rc = -EPERM;
+
+ if (!rc) {
+ data_len = be16_to_cpup(
+ (__be16 *) &buf.data[TPM_HEADER_SIZE + 4]);
+ if (data_len < MIN_KEY_SIZE || data_len > MAX_KEY_SIZE + 1) {
+ rc = -EFAULT;
+ goto out;
+ }
+
+ if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 6 + data_len) {
+ rc = -EFAULT;
+ goto out;
+ }
+ data = &buf.data[TPM_HEADER_SIZE + 6];
+
+ memcpy(payload->key, data, data_len - 1);
+ payload->key_len = data_len - 1;
+ payload->migratable = data[data_len - 1];
+ }
+
+out:
+ tpm_buf_destroy(&buf);
+ return rc;
+}
+
+/**
+ * tpm2_unseal_trusted() - unseal the payload of a trusted key
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ *
+ * Return: Same as with tpm_send.
+ */
+int tpm2_unseal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options)
+{
+ u32 blob_handle;
+ int rc;
+
+ rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
+ if (rc)
+ return rc;
+
+ rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
+
+ return rc;
+}
--
2.7.4
^ permalink raw reply related
* [Patch v8 3/4] KEYS: trusted: Create trusted keys subsystem
From: Sumit Garg @ 2019-10-16 5:14 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1571202895-32651-1-git-send-email-sumit.garg@linaro.org>
Move existing code to trusted keys subsystem. Also, rename files with
"tpm" as suffix which provides the underlying implementation.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
crypto/asymmetric_keys/asym_tpm.c | 2 +-
include/Kbuild | 1 -
include/keys/{trusted.h => trusted_tpm.h} | 7 +++++--
security/keys/Makefile | 2 +-
security/keys/trusted-keys/Makefile | 7 +++++++
security/keys/{trusted.c => trusted-keys/trusted_tpm1.c} | 2 +-
6 files changed, 15 insertions(+), 6 deletions(-)
rename include/keys/{trusted.h => trusted_tpm.h} (96%)
create mode 100644 security/keys/trusted-keys/Makefile
rename security/keys/{trusted.c => trusted-keys/trusted_tpm1.c} (99%)
diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index a2b2a61..d16d893 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -13,7 +13,7 @@
#include <crypto/sha.h>
#include <asm/unaligned.h>
#include <keys/asymmetric-subtype.h>
-#include <keys/trusted.h>
+#include <keys/trusted_tpm.h>
#include <crypto/asym_tpm_subtype.h>
#include <crypto/public_key.h>
diff --git a/include/Kbuild b/include/Kbuild
index ffba794..6f9ec5a 100644
--- a/include/Kbuild
+++ b/include/Kbuild
@@ -65,7 +65,6 @@ header-test- += keys/asymmetric-subtype.h
header-test- += keys/asymmetric-type.h
header-test- += keys/big_key-type.h
header-test- += keys/request_key_auth-type.h
-header-test- += keys/trusted.h
header-test- += kvm/arm_arch_timer.h
header-test- += kvm/arm_pmu.h
header-test-$(CONFIG_ARM) += kvm/arm_psci.h
diff --git a/include/keys/trusted.h b/include/keys/trusted_tpm.h
similarity index 96%
rename from include/keys/trusted.h
rename to include/keys/trusted_tpm.h
index 29e3e9b..7b9d7b4 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted_tpm.h
@@ -1,6 +1,9 @@
/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __TRUSTED_KEY_H
-#define __TRUSTED_KEY_H
+#ifndef __TRUSTED_TPM_H
+#define __TRUSTED_TPM_H
+
+#include <keys/trusted-type.h>
+#include <linux/tpm_command.h>
/* implementation specific TPM constants */
#define MAX_BUF_SIZE 1024
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 9cef540..074f275 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -28,5 +28,5 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
# Key types
#
obj-$(CONFIG_BIG_KEYS) += big_key.o
-obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
+obj-$(CONFIG_TRUSTED_KEYS) += trusted-keys/
obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
new file mode 100644
index 0000000..1a24680
--- /dev/null
+++ b/security/keys/trusted-keys/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for trusted keys
+#
+
+obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
+trusted-y += trusted_tpm1.o
diff --git a/security/keys/trusted.c b/security/keys/trusted-keys/trusted_tpm1.c
similarity index 99%
rename from security/keys/trusted.c
rename to security/keys/trusted-keys/trusted_tpm1.c
index 7071011..e3155fd 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -27,7 +27,7 @@
#include <linux/tpm.h>
#include <linux/tpm_command.h>
-#include <keys/trusted.h>
+#include <keys/trusted_tpm.h>
static const char hmac_alg[] = "hmac(sha1)";
static const char hash_alg[] = "sha1";
--
2.7.4
^ permalink raw reply related
* [Patch v8 2/4] KEYS: Use common tpm_buf for trusted and asymmetric keys
From: Sumit Garg @ 2019-10-16 5:14 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1571202895-32651-1-git-send-email-sumit.garg@linaro.org>
Switch to utilize common heap based tpm_buf code for TPM based trusted
and asymmetric keys rather than using stack based tpm1_buf code. Also,
remove tpm1_buf code.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
crypto/asymmetric_keys/asym_tpm.c | 107 ++++++++++++++++----------------------
include/keys/trusted.h | 37 +------------
security/keys/trusted.c | 98 +++++++++++++++-------------------
3 files changed, 89 insertions(+), 153 deletions(-)
diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index b88968d..a2b2a61 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -21,17 +21,13 @@
#define TPM_ORD_LOADKEY2 65
#define TPM_ORD_UNBIND 30
#define TPM_ORD_SIGN 60
-#define TPM_LOADKEY2_SIZE 59
-#define TPM_FLUSHSPECIFIC_SIZE 18
-#define TPM_UNBIND_SIZE 63
-#define TPM_SIGN_SIZE 63
#define TPM_RT_KEY 0x00000001
/*
* Load a TPM key from the blob provided by userspace
*/
-static int tpm_loadkey2(struct tpm1_buf *tb,
+static int tpm_loadkey2(struct tpm_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *keyblob, int keybloblen,
uint32_t *newhandle)
@@ -68,16 +64,13 @@ static int tpm_loadkey2(struct tpm1_buf *tb,
return ret;
/* build the request buffer */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_LOADKEY2_SIZE + keybloblen);
- store32(tb, TPM_ORD_LOADKEY2);
- store32(tb, keyhandle);
- storebytes(tb, keyblob, keybloblen);
- store32(tb, authhandle);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_LOADKEY2);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append(tb, keyblob, keybloblen);
+ tpm_buf_append_u32(tb, authhandle);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -99,14 +92,11 @@ static int tpm_loadkey2(struct tpm1_buf *tb,
/*
* Execute the FlushSpecific TPM command
*/
-static int tpm_flushspecific(struct tpm1_buf *tb, uint32_t handle)
+static int tpm_flushspecific(struct tpm_buf *tb, uint32_t handle)
{
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_COMMAND);
- store32(tb, TPM_FLUSHSPECIFIC_SIZE);
- store32(tb, TPM_ORD_FLUSHSPECIFIC);
- store32(tb, handle);
- store32(tb, TPM_RT_KEY);
+ tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_FLUSHSPECIFIC);
+ tpm_buf_append_u32(tb, handle);
+ tpm_buf_append_u32(tb, TPM_RT_KEY);
return trusted_tpm_send(tb->data, MAX_BUF_SIZE);
}
@@ -115,7 +105,7 @@ static int tpm_flushspecific(struct tpm1_buf *tb, uint32_t handle)
* Decrypt a blob provided by userspace using a specific key handle.
* The handle is a well known handle or previously loaded by e.g. LoadKey2
*/
-static int tpm_unbind(struct tpm1_buf *tb,
+static int tpm_unbind(struct tpm_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -155,17 +145,14 @@ static int tpm_unbind(struct tpm1_buf *tb,
return ret;
/* build the request buffer */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_UNBIND_SIZE + bloblen);
- store32(tb, TPM_ORD_UNBIND);
- store32(tb, keyhandle);
- store32(tb, bloblen);
- storebytes(tb, blob, bloblen);
- store32(tb, authhandle);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_UNBIND);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append_u32(tb, bloblen);
+ tpm_buf_append(tb, blob, bloblen);
+ tpm_buf_append_u32(tb, authhandle);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -201,7 +188,7 @@ static int tpm_unbind(struct tpm1_buf *tb,
* up to key_length_in_bytes - 11 and not be limited to size 20 like the
* TPM_SS_RSASSAPKCS1v15_SHA1 signature scheme.
*/
-static int tpm_sign(struct tpm1_buf *tb,
+static int tpm_sign(struct tpm_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -241,17 +228,14 @@ static int tpm_sign(struct tpm1_buf *tb,
return ret;
/* build the request buffer */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_SIGN_SIZE + bloblen);
- store32(tb, TPM_ORD_SIGN);
- store32(tb, keyhandle);
- store32(tb, bloblen);
- storebytes(tb, blob, bloblen);
- store32(tb, authhandle);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_SIGN);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append_u32(tb, bloblen);
+ tpm_buf_append(tb, blob, bloblen);
+ tpm_buf_append_u32(tb, authhandle);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -519,7 +503,7 @@ static int tpm_key_decrypt(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
@@ -533,14 +517,14 @@ static int tpm_key_decrypt(struct tpm_key *tk,
if (strcmp(params->encoding, "pkcs1"))
return -ENOPKG;
- tb = kzalloc(sizeof(*tb), GFP_KERNEL);
- if (!tb)
- return -ENOMEM;
+ r = tpm_buf_init(&tb, 0, 0);
+ if (r)
+ return r;
/* TODO: Handle a non-all zero SRK authorization */
memset(srkauth, 0, sizeof(srkauth));
- r = tpm_loadkey2(tb, SRKHANDLE, srkauth,
+ r = tpm_loadkey2(&tb, SRKHANDLE, srkauth,
tk->blob, tk->blob_len, &keyhandle);
if (r < 0) {
pr_devel("loadkey2 failed (%d)\n", r);
@@ -550,16 +534,16 @@ static int tpm_key_decrypt(struct tpm_key *tk,
/* TODO: Handle a non-all zero key authorization */
memset(keyauth, 0, sizeof(keyauth));
- r = tpm_unbind(tb, keyhandle, keyauth,
+ r = tpm_unbind(&tb, keyhandle, keyauth,
in, params->in_len, out, params->out_len);
if (r < 0)
pr_devel("tpm_unbind failed (%d)\n", r);
- if (tpm_flushspecific(tb, keyhandle) < 0)
+ if (tpm_flushspecific(&tb, keyhandle) < 0)
pr_devel("flushspecific failed (%d)\n", r);
error:
- kzfree(tb);
+ tpm_buf_destroy(&tb);
pr_devel("<==%s() = %d\n", __func__, r);
return r;
}
@@ -643,7 +627,7 @@ static int tpm_key_sign(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
@@ -681,15 +665,14 @@ static int tpm_key_sign(struct tpm_key *tk,
goto error_free_asn1_wrapped;
}
- r = -ENOMEM;
- tb = kzalloc(sizeof(*tb), GFP_KERNEL);
- if (!tb)
+ r = tpm_buf_init(&tb, 0, 0);
+ if (r)
goto error_free_asn1_wrapped;
/* TODO: Handle a non-all zero SRK authorization */
memset(srkauth, 0, sizeof(srkauth));
- r = tpm_loadkey2(tb, SRKHANDLE, srkauth,
+ r = tpm_loadkey2(&tb, SRKHANDLE, srkauth,
tk->blob, tk->blob_len, &keyhandle);
if (r < 0) {
pr_devel("loadkey2 failed (%d)\n", r);
@@ -699,15 +682,15 @@ static int tpm_key_sign(struct tpm_key *tk,
/* TODO: Handle a non-all zero key authorization */
memset(keyauth, 0, sizeof(keyauth));
- r = tpm_sign(tb, keyhandle, keyauth, in, in_len, out, params->out_len);
+ r = tpm_sign(&tb, keyhandle, keyauth, in, in_len, out, params->out_len);
if (r < 0)
pr_devel("tpm_sign failed (%d)\n", r);
- if (tpm_flushspecific(tb, keyhandle) < 0)
+ if (tpm_flushspecific(&tb, keyhandle) < 0)
pr_devel("flushspecific failed (%d)\n", r);
error_free_tb:
- kzfree(tb);
+ tpm_buf_destroy(&tb);
error_free_asn1_wrapped:
kfree(asn1_wrapped);
pr_devel("<==%s() = %d\n", __func__, r);
diff --git a/include/keys/trusted.h b/include/keys/trusted.h
index 841ae11..29e3e9b 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted.h
@@ -5,10 +5,6 @@
/* implementation specific TPM constants */
#define MAX_BUF_SIZE 1024
#define TPM_GETRANDOM_SIZE 14
-#define TPM_OSAP_SIZE 36
-#define TPM_OIAP_SIZE 10
-#define TPM_SEAL_SIZE 87
-#define TPM_UNSEAL_SIZE 104
#define TPM_SIZE_OFFSET 2
#define TPM_RETURN_OFFSET 6
#define TPM_DATA_OFFSET 10
@@ -17,13 +13,6 @@
#define LOAD32N(buffer, offset) (*(uint32_t *)&buffer[offset])
#define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
-struct tpm1_buf {
- int len;
- unsigned char data[MAX_BUF_SIZE];
-};
-
-#define INIT_BUF(tb) (tb->len = 0)
-
struct osapsess {
uint32_t handle;
unsigned char secret[SHA1_DIGEST_SIZE];
@@ -46,7 +35,7 @@ int TSS_checkhmac1(unsigned char *buffer,
unsigned int keylen, ...);
int trusted_tpm_send(unsigned char *cmd, size_t buflen);
-int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce);
+int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce);
#define TPM_DEBUG 0
@@ -109,28 +98,4 @@ static inline void dump_tpm_buf(unsigned char *buf)
{
}
#endif
-
-static inline void store8(struct tpm1_buf *buf, const unsigned char value)
-{
- buf->data[buf->len++] = value;
-}
-
-static inline void store16(struct tpm1_buf *buf, const uint16_t value)
-{
- *(uint16_t *) & buf->data[buf->len] = htons(value);
- buf->len += sizeof value;
-}
-
-static inline void store32(struct tpm1_buf *buf, const uint32_t value)
-{
- *(uint32_t *) & buf->data[buf->len] = htonl(value);
- buf->len += sizeof value;
-}
-
-static inline void storebytes(struct tpm1_buf *buf, const unsigned char *in,
- const int len)
-{
- memcpy(buf->data + buf->len, in, len);
- buf->len += len;
-}
#endif
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 4cfae208..7071011 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -395,7 +395,7 @@ static int pcrlock(const int pcrnum)
/*
* Create an object specific authorisation protocol (OSAP) session
*/
-static int osap(struct tpm1_buf *tb, struct osapsess *s,
+static int osap(struct tpm_buf *tb, struct osapsess *s,
const unsigned char *key, uint16_t type, uint32_t handle)
{
unsigned char enonce[TPM_NONCE_SIZE];
@@ -406,13 +406,10 @@ static int osap(struct tpm1_buf *tb, struct osapsess *s,
if (ret != TPM_NONCE_SIZE)
return ret;
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_COMMAND);
- store32(tb, TPM_OSAP_SIZE);
- store32(tb, TPM_ORD_OSAP);
- store16(tb, type);
- store32(tb, handle);
- storebytes(tb, ononce, TPM_NONCE_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_OSAP);
+ tpm_buf_append_u16(tb, type);
+ tpm_buf_append_u32(tb, handle);
+ tpm_buf_append(tb, ononce, TPM_NONCE_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
@@ -430,17 +427,14 @@ static int osap(struct tpm1_buf *tb, struct osapsess *s,
/*
* Create an object independent authorisation protocol (oiap) session
*/
-int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce)
+int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
{
int ret;
if (!chip)
return -ENODEV;
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_COMMAND);
- store32(tb, TPM_OIAP_SIZE);
- store32(tb, TPM_ORD_OIAP);
+ tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_OIAP);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
return ret;
@@ -464,7 +458,7 @@ struct tpm_digests {
* Have the TPM seal(encrypt) the trusted key, possibly based on
* Platform Configuration Registers (PCRs). AUTH1 for sealing key.
*/
-static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
+static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *data, uint32_t datalen,
unsigned char *blob, uint32_t *bloblen,
@@ -535,20 +529,17 @@ static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
goto out;
/* build and send the TPM request packet */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_SEAL_SIZE + pcrinfosize + datalen);
- store32(tb, TPM_ORD_SEAL);
- store32(tb, keyhandle);
- storebytes(tb, td->encauth, SHA1_DIGEST_SIZE);
- store32(tb, pcrinfosize);
- storebytes(tb, pcrinfo, pcrinfosize);
- store32(tb, datalen);
- storebytes(tb, data, datalen);
- store32(tb, sess.handle);
- storebytes(tb, td->nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, td->pubauth, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_SEAL);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append(tb, td->encauth, SHA1_DIGEST_SIZE);
+ tpm_buf_append_u32(tb, pcrinfosize);
+ tpm_buf_append(tb, pcrinfo, pcrinfosize);
+ tpm_buf_append_u32(tb, datalen);
+ tpm_buf_append(tb, data, datalen);
+ tpm_buf_append_u32(tb, sess.handle);
+ tpm_buf_append(tb, td->nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, td->pubauth, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
@@ -579,7 +570,7 @@ static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
/*
* use the AUTH2_COMMAND form of unseal, to authorize both key and blob
*/
-static int tpm_unseal(struct tpm1_buf *tb,
+static int tpm_unseal(struct tpm_buf *tb,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *blob, int bloblen,
const unsigned char *blobauth,
@@ -628,20 +619,17 @@ static int tpm_unseal(struct tpm1_buf *tb,
return ret;
/* build and send TPM request packet */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH2_COMMAND);
- store32(tb, TPM_UNSEAL_SIZE + bloblen);
- store32(tb, TPM_ORD_UNSEAL);
- store32(tb, keyhandle);
- storebytes(tb, blob, bloblen);
- store32(tb, authhandle1);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata1, SHA1_DIGEST_SIZE);
- store32(tb, authhandle2);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata2, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH2_COMMAND, TPM_ORD_UNSEAL);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append(tb, blob, bloblen);
+ tpm_buf_append_u32(tb, authhandle1);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata1, SHA1_DIGEST_SIZE);
+ tpm_buf_append_u32(tb, authhandle2);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata2, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -670,23 +658,23 @@ static int tpm_unseal(struct tpm1_buf *tb,
static int key_seal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
int ret;
- tb = kzalloc(sizeof *tb, GFP_KERNEL);
- if (!tb)
- return -ENOMEM;
+ ret = tpm_buf_init(&tb, 0, 0);
+ if (ret)
+ return ret;
/* include migratable flag at end of sealed key */
p->key[p->key_len] = p->migratable;
- ret = tpm_seal(tb, o->keytype, o->keyhandle, o->keyauth,
+ ret = tpm_seal(&tb, o->keytype, o->keyhandle, o->keyauth,
p->key, p->key_len + 1, p->blob, &p->blob_len,
o->blobauth, o->pcrinfo, o->pcrinfo_len);
if (ret < 0)
pr_info("trusted_key: srkseal failed (%d)\n", ret);
- kzfree(tb);
+ tpm_buf_destroy(&tb);
return ret;
}
@@ -696,14 +684,14 @@ static int key_seal(struct trusted_key_payload *p,
static int key_unseal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
int ret;
- tb = kzalloc(sizeof *tb, GFP_KERNEL);
- if (!tb)
- return -ENOMEM;
+ ret = tpm_buf_init(&tb, 0, 0);
+ if (ret)
+ return ret;
- ret = tpm_unseal(tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
+ ret = tpm_unseal(&tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
o->blobauth, p->key, &p->key_len);
if (ret < 0)
pr_info("trusted_key: srkunseal failed (%d)\n", ret);
@@ -711,7 +699,7 @@ static int key_unseal(struct trusted_key_payload *p,
/* pull migratable flag out of sealed key */
p->migratable = p->key[--p->key_len];
- kzfree(tb);
+ tpm_buf_destroy(&tb);
return ret;
}
--
2.7.4
^ permalink raw reply related
* [Patch v8 1/4] tpm: Move tpm_buf code to include/linux/
From: Sumit Garg @ 2019-10-16 5:14 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1571202895-32651-1-git-send-email-sumit.garg@linaro.org>
Move tpm_buf code to common include/linux/tpm.h header so that it can
be reused via other subsystems like trusted keys etc.
Also rename trusted keys and asymmetric keys usage of TPM 1.x buffer
implementation to tpm1_buf to avoid any compilation errors.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
---
crypto/asymmetric_keys/asym_tpm.c | 12 +--
drivers/char/tpm/tpm.h | 212 --------------------------------------
include/keys/trusted.h | 12 +--
include/linux/tpm.h | 212 ++++++++++++++++++++++++++++++++++++++
security/keys/trusted.c | 12 +--
5 files changed, 230 insertions(+), 230 deletions(-)
diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index 76d2ce3..b88968d 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -31,7 +31,7 @@
/*
* Load a TPM key from the blob provided by userspace
*/
-static int tpm_loadkey2(struct tpm_buf *tb,
+static int tpm_loadkey2(struct tpm1_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *keyblob, int keybloblen,
uint32_t *newhandle)
@@ -99,7 +99,7 @@ static int tpm_loadkey2(struct tpm_buf *tb,
/*
* Execute the FlushSpecific TPM command
*/
-static int tpm_flushspecific(struct tpm_buf *tb, uint32_t handle)
+static int tpm_flushspecific(struct tpm1_buf *tb, uint32_t handle)
{
INIT_BUF(tb);
store16(tb, TPM_TAG_RQU_COMMAND);
@@ -115,7 +115,7 @@ static int tpm_flushspecific(struct tpm_buf *tb, uint32_t handle)
* Decrypt a blob provided by userspace using a specific key handle.
* The handle is a well known handle or previously loaded by e.g. LoadKey2
*/
-static int tpm_unbind(struct tpm_buf *tb,
+static int tpm_unbind(struct tpm1_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -201,7 +201,7 @@ static int tpm_unbind(struct tpm_buf *tb,
* up to key_length_in_bytes - 11 and not be limited to size 20 like the
* TPM_SS_RSASSAPKCS1v15_SHA1 signature scheme.
*/
-static int tpm_sign(struct tpm_buf *tb,
+static int tpm_sign(struct tpm1_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -519,7 +519,7 @@ static int tpm_key_decrypt(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
@@ -643,7 +643,7 @@ static int tpm_key_sign(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 0d1fd37..b174cf4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -25,7 +25,6 @@
#include <linux/platform_device.h>
#include <linux/io.h>
#include <linux/tpm.h>
-#include <linux/highmem.h>
#include <linux/tpm_eventlog.h>
#ifdef CONFIG_X86
@@ -58,124 +57,6 @@ enum tpm_addr {
#define TPM_ERR_DISABLED 0x7
#define TPM_ERR_INVALID_POSTINIT 38
-#define TPM_HEADER_SIZE 10
-
-enum tpm2_const {
- TPM2_PLATFORM_PCR = 24,
- TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
-};
-
-enum tpm2_timeouts {
- TPM2_TIMEOUT_A = 750,
- TPM2_TIMEOUT_B = 2000,
- TPM2_TIMEOUT_C = 200,
- TPM2_TIMEOUT_D = 30,
- TPM2_DURATION_SHORT = 20,
- TPM2_DURATION_MEDIUM = 750,
- TPM2_DURATION_LONG = 2000,
- TPM2_DURATION_LONG_LONG = 300000,
- TPM2_DURATION_DEFAULT = 120000,
-};
-
-enum tpm2_structures {
- TPM2_ST_NO_SESSIONS = 0x8001,
- TPM2_ST_SESSIONS = 0x8002,
-};
-
-/* Indicates from what layer of the software stack the error comes from */
-#define TSS2_RC_LAYER_SHIFT 16
-#define TSS2_RESMGR_TPM_RC_LAYER (11 << TSS2_RC_LAYER_SHIFT)
-
-enum tpm2_return_codes {
- TPM2_RC_SUCCESS = 0x0000,
- TPM2_RC_HASH = 0x0083, /* RC_FMT1 */
- TPM2_RC_HANDLE = 0x008B,
- TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
- TPM2_RC_FAILURE = 0x0101,
- TPM2_RC_DISABLED = 0x0120,
- TPM2_RC_COMMAND_CODE = 0x0143,
- TPM2_RC_TESTING = 0x090A, /* RC_WARN */
- TPM2_RC_REFERENCE_H0 = 0x0910,
- TPM2_RC_RETRY = 0x0922,
-};
-
-enum tpm2_command_codes {
- TPM2_CC_FIRST = 0x011F,
- TPM2_CC_HIERARCHY_CONTROL = 0x0121,
- TPM2_CC_HIERARCHY_CHANGE_AUTH = 0x0129,
- TPM2_CC_CREATE_PRIMARY = 0x0131,
- TPM2_CC_SEQUENCE_COMPLETE = 0x013E,
- TPM2_CC_SELF_TEST = 0x0143,
- TPM2_CC_STARTUP = 0x0144,
- TPM2_CC_SHUTDOWN = 0x0145,
- TPM2_CC_NV_READ = 0x014E,
- TPM2_CC_CREATE = 0x0153,
- TPM2_CC_LOAD = 0x0157,
- TPM2_CC_SEQUENCE_UPDATE = 0x015C,
- TPM2_CC_UNSEAL = 0x015E,
- TPM2_CC_CONTEXT_LOAD = 0x0161,
- TPM2_CC_CONTEXT_SAVE = 0x0162,
- TPM2_CC_FLUSH_CONTEXT = 0x0165,
- TPM2_CC_VERIFY_SIGNATURE = 0x0177,
- TPM2_CC_GET_CAPABILITY = 0x017A,
- TPM2_CC_GET_RANDOM = 0x017B,
- TPM2_CC_PCR_READ = 0x017E,
- TPM2_CC_PCR_EXTEND = 0x0182,
- TPM2_CC_EVENT_SEQUENCE_COMPLETE = 0x0185,
- TPM2_CC_HASH_SEQUENCE_START = 0x0186,
- TPM2_CC_CREATE_LOADED = 0x0191,
- TPM2_CC_LAST = 0x0193, /* Spec 1.36 */
-};
-
-enum tpm2_permanent_handles {
- TPM2_RS_PW = 0x40000009,
-};
-
-enum tpm2_capabilities {
- TPM2_CAP_HANDLES = 1,
- TPM2_CAP_COMMANDS = 2,
- TPM2_CAP_PCRS = 5,
- TPM2_CAP_TPM_PROPERTIES = 6,
-};
-
-enum tpm2_properties {
- TPM_PT_TOTAL_COMMANDS = 0x0129,
-};
-
-enum tpm2_startup_types {
- TPM2_SU_CLEAR = 0x0000,
- TPM2_SU_STATE = 0x0001,
-};
-
-enum tpm2_cc_attrs {
- TPM2_CC_ATTR_CHANDLES = 25,
- TPM2_CC_ATTR_RHANDLE = 28,
-};
-
-#define TPM_VID_INTEL 0x8086
-#define TPM_VID_WINBOND 0x1050
-#define TPM_VID_STM 0x104A
-
-enum tpm_chip_flags {
- TPM_CHIP_FLAG_TPM2 = BIT(1),
- TPM_CHIP_FLAG_IRQ = BIT(2),
- TPM_CHIP_FLAG_VIRTUAL = BIT(3),
- TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
- TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
- TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED = BIT(6),
-};
-
-#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
-
-struct tpm_header {
- __be16 tag;
- __be32 length;
- union {
- __be32 ordinal;
- __be32 return_code;
- };
-} __packed;
-
#define TPM_TAG_RQU_COMMAND 193
struct stclear_flags_t {
@@ -272,99 +153,6 @@ enum tpm_sub_capabilities {
* compiler warnings about stack frame size. */
#define TPM_MAX_RNG_DATA 128
-/* A string buffer type for constructing TPM commands. This is based on the
- * ideas of string buffer code in security/keys/trusted.h but is heap based
- * in order to keep the stack usage minimal.
- */
-
-enum tpm_buf_flags {
- TPM_BUF_OVERFLOW = BIT(0),
-};
-
-struct tpm_buf {
- unsigned int flags;
- u8 *data;
-};
-
-static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- head->tag = cpu_to_be16(tag);
- head->length = cpu_to_be32(sizeof(*head));
- head->ordinal = cpu_to_be32(ordinal);
-}
-
-static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- buf->data = (u8 *)__get_free_page(GFP_KERNEL);
- if (!buf->data)
- return -ENOMEM;
-
- buf->flags = 0;
- tpm_buf_reset(buf, tag, ordinal);
- return 0;
-}
-
-static inline void tpm_buf_destroy(struct tpm_buf *buf)
-{
- free_page((unsigned long)buf->data);
-}
-
-static inline u32 tpm_buf_length(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be32_to_cpu(head->length);
-}
-
-static inline u16 tpm_buf_tag(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be16_to_cpu(head->tag);
-}
-
-static inline void tpm_buf_append(struct tpm_buf *buf,
- const unsigned char *new_data,
- unsigned int new_len)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
- u32 len = tpm_buf_length(buf);
-
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_OVERFLOW)
- return;
-
- if ((len + new_len) > PAGE_SIZE) {
- WARN(1, "tpm_buf: overflow\n");
- buf->flags |= TPM_BUF_OVERFLOW;
- return;
- }
-
- memcpy(&buf->data[len], new_data, new_len);
- head->length = cpu_to_be32(len + new_len);
-}
-
-static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
-{
- tpm_buf_append(buf, &value, 1);
-}
-
-static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
-{
- __be16 value2 = cpu_to_be16(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 2);
-}
-
-static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
-{
- __be32 value2 = cpu_to_be32(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 4);
-}
-
extern struct class *tpm_class;
extern struct class *tpmrm_class;
extern dev_t tpm_devt;
diff --git a/include/keys/trusted.h b/include/keys/trusted.h
index 0071298..841ae11 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted.h
@@ -17,7 +17,7 @@
#define LOAD32N(buffer, offset) (*(uint32_t *)&buffer[offset])
#define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
-struct tpm_buf {
+struct tpm1_buf {
int len;
unsigned char data[MAX_BUF_SIZE];
};
@@ -46,7 +46,7 @@ int TSS_checkhmac1(unsigned char *buffer,
unsigned int keylen, ...);
int trusted_tpm_send(unsigned char *cmd, size_t buflen);
-int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce);
+int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce);
#define TPM_DEBUG 0
@@ -110,24 +110,24 @@ static inline void dump_tpm_buf(unsigned char *buf)
}
#endif
-static inline void store8(struct tpm_buf *buf, const unsigned char value)
+static inline void store8(struct tpm1_buf *buf, const unsigned char value)
{
buf->data[buf->len++] = value;
}
-static inline void store16(struct tpm_buf *buf, const uint16_t value)
+static inline void store16(struct tpm1_buf *buf, const uint16_t value)
{
*(uint16_t *) & buf->data[buf->len] = htons(value);
buf->len += sizeof value;
}
-static inline void store32(struct tpm_buf *buf, const uint32_t value)
+static inline void store32(struct tpm1_buf *buf, const uint32_t value)
{
*(uint32_t *) & buf->data[buf->len] = htonl(value);
buf->len += sizeof value;
}
-static inline void storebytes(struct tpm_buf *buf, const unsigned char *in,
+static inline void storebytes(struct tpm1_buf *buf, const unsigned char *in,
const int len)
{
memcpy(buf->data + buf->len, in, len);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index bb1d1ac..c78119f 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -21,6 +21,7 @@
#include <linux/acpi.h>
#include <linux/cdev.h>
#include <linux/fs.h>
+#include <linux/highmem.h>
#include <crypto/hash_info.h>
#define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
@@ -163,6 +164,217 @@ struct tpm_chip {
int locality;
};
+#define TPM_HEADER_SIZE 10
+
+enum tpm2_const {
+ TPM2_PLATFORM_PCR = 24,
+ TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
+};
+
+enum tpm2_timeouts {
+ TPM2_TIMEOUT_A = 750,
+ TPM2_TIMEOUT_B = 2000,
+ TPM2_TIMEOUT_C = 200,
+ TPM2_TIMEOUT_D = 30,
+ TPM2_DURATION_SHORT = 20,
+ TPM2_DURATION_MEDIUM = 750,
+ TPM2_DURATION_LONG = 2000,
+ TPM2_DURATION_LONG_LONG = 300000,
+ TPM2_DURATION_DEFAULT = 120000,
+};
+
+enum tpm2_structures {
+ TPM2_ST_NO_SESSIONS = 0x8001,
+ TPM2_ST_SESSIONS = 0x8002,
+};
+
+/* Indicates from what layer of the software stack the error comes from */
+#define TSS2_RC_LAYER_SHIFT 16
+#define TSS2_RESMGR_TPM_RC_LAYER (11 << TSS2_RC_LAYER_SHIFT)
+
+enum tpm2_return_codes {
+ TPM2_RC_SUCCESS = 0x0000,
+ TPM2_RC_HASH = 0x0083, /* RC_FMT1 */
+ TPM2_RC_HANDLE = 0x008B,
+ TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
+ TPM2_RC_FAILURE = 0x0101,
+ TPM2_RC_DISABLED = 0x0120,
+ TPM2_RC_COMMAND_CODE = 0x0143,
+ TPM2_RC_TESTING = 0x090A, /* RC_WARN */
+ TPM2_RC_REFERENCE_H0 = 0x0910,
+ TPM2_RC_RETRY = 0x0922,
+};
+
+enum tpm2_command_codes {
+ TPM2_CC_FIRST = 0x011F,
+ TPM2_CC_HIERARCHY_CONTROL = 0x0121,
+ TPM2_CC_HIERARCHY_CHANGE_AUTH = 0x0129,
+ TPM2_CC_CREATE_PRIMARY = 0x0131,
+ TPM2_CC_SEQUENCE_COMPLETE = 0x013E,
+ TPM2_CC_SELF_TEST = 0x0143,
+ TPM2_CC_STARTUP = 0x0144,
+ TPM2_CC_SHUTDOWN = 0x0145,
+ TPM2_CC_NV_READ = 0x014E,
+ TPM2_CC_CREATE = 0x0153,
+ TPM2_CC_LOAD = 0x0157,
+ TPM2_CC_SEQUENCE_UPDATE = 0x015C,
+ TPM2_CC_UNSEAL = 0x015E,
+ TPM2_CC_CONTEXT_LOAD = 0x0161,
+ TPM2_CC_CONTEXT_SAVE = 0x0162,
+ TPM2_CC_FLUSH_CONTEXT = 0x0165,
+ TPM2_CC_VERIFY_SIGNATURE = 0x0177,
+ TPM2_CC_GET_CAPABILITY = 0x017A,
+ TPM2_CC_GET_RANDOM = 0x017B,
+ TPM2_CC_PCR_READ = 0x017E,
+ TPM2_CC_PCR_EXTEND = 0x0182,
+ TPM2_CC_EVENT_SEQUENCE_COMPLETE = 0x0185,
+ TPM2_CC_HASH_SEQUENCE_START = 0x0186,
+ TPM2_CC_CREATE_LOADED = 0x0191,
+ TPM2_CC_LAST = 0x0193, /* Spec 1.36 */
+};
+
+enum tpm2_permanent_handles {
+ TPM2_RS_PW = 0x40000009,
+};
+
+enum tpm2_capabilities {
+ TPM2_CAP_HANDLES = 1,
+ TPM2_CAP_COMMANDS = 2,
+ TPM2_CAP_PCRS = 5,
+ TPM2_CAP_TPM_PROPERTIES = 6,
+};
+
+enum tpm2_properties {
+ TPM_PT_TOTAL_COMMANDS = 0x0129,
+};
+
+enum tpm2_startup_types {
+ TPM2_SU_CLEAR = 0x0000,
+ TPM2_SU_STATE = 0x0001,
+};
+
+enum tpm2_cc_attrs {
+ TPM2_CC_ATTR_CHANDLES = 25,
+ TPM2_CC_ATTR_RHANDLE = 28,
+};
+
+#define TPM_VID_INTEL 0x8086
+#define TPM_VID_WINBOND 0x1050
+#define TPM_VID_STM 0x104A
+
+enum tpm_chip_flags {
+ TPM_CHIP_FLAG_TPM2 = BIT(1),
+ TPM_CHIP_FLAG_IRQ = BIT(2),
+ TPM_CHIP_FLAG_VIRTUAL = BIT(3),
+ TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
+ TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
+ TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED = BIT(6),
+};
+
+#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
+
+struct tpm_header {
+ __be16 tag;
+ __be32 length;
+ union {
+ __be32 ordinal;
+ __be32 return_code;
+ };
+} __packed;
+
+/* A string buffer type for constructing TPM commands. This is based on the
+ * ideas of string buffer code in security/keys/trusted.h but is heap based
+ * in order to keep the stack usage minimal.
+ */
+
+enum tpm_buf_flags {
+ TPM_BUF_OVERFLOW = BIT(0),
+};
+
+struct tpm_buf {
+ unsigned int flags;
+ u8 *data;
+};
+
+static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ head->tag = cpu_to_be16(tag);
+ head->length = cpu_to_be32(sizeof(*head));
+ head->ordinal = cpu_to_be32(ordinal);
+}
+
+static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ buf->data = (u8 *)__get_free_page(GFP_KERNEL);
+ if (!buf->data)
+ return -ENOMEM;
+
+ buf->flags = 0;
+ tpm_buf_reset(buf, tag, ordinal);
+ return 0;
+}
+
+static inline void tpm_buf_destroy(struct tpm_buf *buf)
+{
+ free_page((unsigned long)buf->data);
+}
+
+static inline u32 tpm_buf_length(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ return be32_to_cpu(head->length);
+}
+
+static inline u16 tpm_buf_tag(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ return be16_to_cpu(head->tag);
+}
+
+static inline void tpm_buf_append(struct tpm_buf *buf,
+ const unsigned char *new_data,
+ unsigned int new_len)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+ u32 len = tpm_buf_length(buf);
+
+ /* Return silently if overflow has already happened. */
+ if (buf->flags & TPM_BUF_OVERFLOW)
+ return;
+
+ if ((len + new_len) > PAGE_SIZE) {
+ WARN(1, "tpm_buf: overflow\n");
+ buf->flags |= TPM_BUF_OVERFLOW;
+ return;
+ }
+
+ memcpy(&buf->data[len], new_data, new_len);
+ head->length = cpu_to_be32(len + new_len);
+}
+
+static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
+{
+ tpm_buf_append(buf, &value, 1);
+}
+
+static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
+{
+ __be16 value2 = cpu_to_be16(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 2);
+}
+
+static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 4);
+}
+
#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
extern int tpm_is_tpm2(struct tpm_chip *chip);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 1fbd778..4cfae208 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -395,7 +395,7 @@ static int pcrlock(const int pcrnum)
/*
* Create an object specific authorisation protocol (OSAP) session
*/
-static int osap(struct tpm_buf *tb, struct osapsess *s,
+static int osap(struct tpm1_buf *tb, struct osapsess *s,
const unsigned char *key, uint16_t type, uint32_t handle)
{
unsigned char enonce[TPM_NONCE_SIZE];
@@ -430,7 +430,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
/*
* Create an object independent authorisation protocol (oiap) session
*/
-int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
+int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce)
{
int ret;
@@ -464,7 +464,7 @@ struct tpm_digests {
* Have the TPM seal(encrypt) the trusted key, possibly based on
* Platform Configuration Registers (PCRs). AUTH1 for sealing key.
*/
-static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
+static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *data, uint32_t datalen,
unsigned char *blob, uint32_t *bloblen,
@@ -579,7 +579,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
/*
* use the AUTH2_COMMAND form of unseal, to authorize both key and blob
*/
-static int tpm_unseal(struct tpm_buf *tb,
+static int tpm_unseal(struct tpm1_buf *tb,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *blob, int bloblen,
const unsigned char *blobauth,
@@ -670,7 +670,7 @@ static int tpm_unseal(struct tpm_buf *tb,
static int key_seal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
int ret;
tb = kzalloc(sizeof *tb, GFP_KERNEL);
@@ -696,7 +696,7 @@ static int key_seal(struct trusted_key_payload *p,
static int key_unseal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
int ret;
tb = kzalloc(sizeof *tb, GFP_KERNEL);
--
2.7.4
^ permalink raw reply related
* [Patch v8 0/4] Create and consolidate trusted keys subsystem
From: Sumit Garg @ 2019-10-16 5:14 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
This patch-set does restructuring of trusted keys code to create and
consolidate trusted keys subsystem.
Also, patch #2 replaces tpm1_buf code used in security/keys/trusted.c and
crypto/asymmertic_keys/asym_tpm.c files to use the common tpm_buf code.
Changes in v8:
1. Rebased to latest tpmdd/master.
2. Added Reviewed-by tags.
Changes in v7:
1. Rebased to top of tpmdd/master
2. Patch #4: update tpm2 trusted keys code to use tpm_send() instead of
tpm_transmit_cmd() which is an internal function.
Changes in v6:
1. Switch TPM asymmetric code also to use common tpm_buf code. These
changes required patches #1 and #2 update, so I have dropped review
tags from those patches.
2. Incorporated miscellaneous comments from Jarkko.
Changes in v5:
1. Drop 5/5 patch as its more relavant along with TEE patch-set.
2. Add Reviewed-by tag for patch #2.
3. Fix build failure when "CONFIG_HEADER_TEST" and
"CONFIG_KERNEL_HEADER_TEST" config options are enabled.
4. Misc changes to rename files.
Changes in v4:
1. Separate patch for export of tpm_buf code to include/linux/tpm.h
2. Change TPM1.x trusted keys code to use common tpm_buf
3. Keep module name as trusted.ko only
Changes in v3:
Move TPM2 trusted keys code to trusted keys subsystem.
Changes in v2:
Split trusted keys abstraction patch for ease of review.
Sumit Garg (4):
tpm: Move tpm_buf code to include/linux/
KEYS: Use common tpm_buf for trusted and asymmetric keys
KEYS: trusted: Create trusted keys subsystem
KEYS: trusted: Move TPM2 trusted keys code
crypto/asymmetric_keys/asym_tpm.c | 101 +++----
drivers/char/tpm/tpm-interface.c | 56 ----
drivers/char/tpm/tpm.h | 223 ---------------
drivers/char/tpm/tpm2-cmd.c | 307 --------------------
include/Kbuild | 1 -
include/keys/{trusted.h => trusted_tpm.h} | 49 +---
include/linux/tpm.h | 248 ++++++++++++++--
security/keys/Makefile | 2 +-
security/keys/trusted-keys/Makefile | 8 +
.../{trusted.c => trusted-keys/trusted_tpm1.c} | 96 +++----
security/keys/trusted-keys/trusted_tpm2.c | 314 +++++++++++++++++++++
11 files changed, 649 insertions(+), 756 deletions(-)
rename include/keys/{trusted.h => trusted_tpm.h} (77%)
create mode 100644 security/keys/trusted-keys/Makefile
rename security/keys/{trusted.c => trusted-keys/trusted_tpm1.c} (94%)
create mode 100644 security/keys/trusted-keys/trusted_tpm2.c
--
2.7.4
^ 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