Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] p9caps: add Plan9 capability devices
To:     Richard Weinberger <richard.weinberger@gmail.com>,
        "Enrico Weigelt, metux IT consult" <metux@gmx.de>
Cc:     LKML <linux-kernel@vger.kernel.org>
References: <40d4c871-a16a-7b8f-2d4a-422a5a490693@infradead.org>
 <20180211215028.16210-1-metux@gmx.de> <20180211215028.16210-2-metux@gmx.de>
 <CAFLxGvwHebXuQYwOdG_vnYfjwtdMfCHg5srzjsj1fEaCFm2QGw@mail.gmail.com>
From:   Enrico Weigelt <lkml@metux.net>
Organization: metux IT consult
Message-ID: <3ffb5399-46b8-7279-fda2-2ca5b40c04d3@metux.net>
Date:   Wed, 25 Apr 2018 12:38:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <CAFLxGvwHebXuQYwOdG_vnYfjwtdMfCHg5srzjsj1fEaCFm2QGw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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