Messages in this thread | | | Subject | Re: [PATCH] p9caps: add Plan9 capability devices | From | Enrico Weigelt <> | Date | Wed, 25 Apr 2018 12:38:02 +0200 |
| |
On 17.02.2018 23:11, Richard Weinberger wrote:
Hi,
>> +static LIST_HEAD(caphash_writers); >> +>> +static DEFINE_MUTEX(lock);>> +>> +struct crypto_ahash *hmac_tfm = NULL;>> +>> +static int caphash_open(struct inode *inode, struct file *filp)>> +{>> + struct caphash_writer *tmp = NULL;>> + struct user_namespace *user_ns = current_user_ns();>> + int retval = 0;>> + struct list_head *pos, *q;>> +>> + /* make sure only one instance per namespace can be opened */>> + mutex_lock(&lock);>> +>> + list_for_each_safe(pos, q, &(caphash_writers)) {>> + tmp = list_entry(pos, struct caphash_writer, list);>> + if (tmp->user_ns == user_ns) {>> + pr_err("already locked in this namespace\n");> > So, evil guy opens the device but does not close it, therefore the > whole service is blocked in a namespace?
Yes, exactly as specified. There may be only one host factotum running, which can create caps. It's an important security feature.
> In general, I think we should open that device in > kernel_init_freeable() and hand over the fd to init/systemd.
That would require an customized init and factotum, and wouldn't be Plan9 compatible.
>> +static int caphash_release(struct inode *inode, struct file *filp) >> +{ >> + int retval = 0; >> + struct user_namespace *user_ns = current_user_ns(); > > Why not obtaining the user namespace from the open file? > That way one can close a caphash file hande she never opened. > Think of open, followed by nsenter, ...
hmm, good point.
>> + list_for_each_safe(pos, q, &(caphash_writers)) { >> + tmp = list_entry(pos, struct caphash_entry, list); > > list_for_each_entry.
what's the exact difference ?
>> +static ssize_t capuse_write(struct file *filp, const char __user *buf, >> + size_t count, loff_t *f_pos) >> +{ >> + ssize_t retval = count; >> + char *rand_str, *src_uname, *dst_uname; >> + u8 hashval[SHA1_DIGEST_SIZE] = { 0 }; >> + char *cmdbuf; >> + >> + if (!(cmdbuf = kzalloc(count, GFP_KERNEL))) > > count is unbound, please apply a limit check.
ok.
>> + { >> + char *walk = cmdbuf; > > cmdbuf is toxic, make sure it is at least nul-terminated.
ok.
>> + if (hmac_tfm) > > IS_ERR()? Otherwise you free a error value, if crypto_alloc_ahash() fails.
ok
--mtx
-- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
| |