Message-ID: <e4116e4f-3a74-f37b-1aa7-4d71aae680d6@gmail.com>
Date:   Mon, 26 Feb 2018 10:25:57 -0800
From:   J Freyensee <why2jjj.linux@...il.com>
To:     Igor Stoppa <igor.stoppa@...wei.com>, david@...morbit.com,
        willy@...radead.org, keescook@...omium.org, mhocko@...nel.org
Cc:     labbott@...hat.com, linux-security-module@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH 4/7] Protectable Memory

inlined responses.


On 2/26/18 6:28 AM, Igor Stoppa wrote:
>
> On 24/02/18 02:10, J Freyensee wrote:
>> On 2/23/18 6:48 AM, Igor Stoppa wrote:
> [...]
>
>>> +struct gen_pool *pmalloc_create_pool(const char *name,
>>> +					 int min_alloc_order);
>> Same comments as earlier.  If this is new API with new code being
>> introduced into the kernel, can the variables be declared to avoid weird
>> problems?  Like min_alloc_order being a negative value makes little
>> sense (based on the description here), so can it be declared as size_t
>> or unsigned int?
> in this case, yes, but I see it as different case

OK.

>
> [...]
>
>>> + * * NULL				- either no memory available or
>>> + *					  pool already read-only
>>> + */
>> I don't know if an errno value is being set, but setting a variable
>> somewhere using EROFS or ENOMEM would more accurate diagnose those two
>> NULL conditions.
> I expect that the latter is highly unlikely to happen, because the user
> of the API controls if the pool is locked or not.
>
> I think it shouldn't come as a surprise to the one who locked the pool,
> if the pool is locked.
>
> If the pool is used with concurrent users, attention should be paid to
> not lock it before ever user is happy (this is where the user of the API
> has to provide own locking.)
>
> Since the information if the pool is already protected is actually
> present in the pmalloc_data structure associated with the pool, I was
> tempted to make it available through the API, but that seemed wrong.
>
> The user of the API should be very aware of the state of the pool, since
> the user is the one who sets it. Why would it have to be read back?

OK, sounds good :-).

>
>>> +void *pmalloc(struct gen_pool *pool, size_t size, gfp_t gfp);
>>> +
>>> +
>>> +/**
>>> + * pzalloc() - zero-initialized version of pmalloc
>>> + * @pool: handle to the pool to be used for memory allocation
>>> + * @size: amount of memory (in bytes) requested
>>> + * @gfp: flags for page allocation
>>> + *
>>> + * Executes pmalloc, initializing the memory requested to 0,
>>> + * before returning the pointer to it.
>>> + *
>>> + * Return:
>>> + * * pointer to the memory requested	- success
>>> + * * NULL				- either no memory available or
>>> + *					  pool already read-only
>>> + */
>> Same comment here, though that inline function below looks highly
>> optimized...
> The same applies here.
> I'm not very fond of the idea of returning the status elsewhere and in a
> way that is not intrinsically connected with the action that has
> determined the change of state.
>
> AFAIK *alloc functions return either the memory requested or NULL.
> I wonder how realistic this case is.

OK.

>
> [...]
>
>>> +	if (unlikely(!(pool && n && size)))
>> Has this code been run through sparse?
> I use "make C=1 W=1"

OK.

>
>> I know one thing sparse looks at
>> is if NULL is being treated like a 0, and sparse does check cases when 0
>> is being used in place for NULL for pointer checks, and I'm wondering if
>> that line of code would pass.
> It's a logical AND: wouldn't NULL translate to false, rather 0?
> I can add an explicit check against NULL, it's probably more readable
> too, but I don't think that the current construct treats NULL as 0.

If it passes sparse, I think it's fine.

>
> [...]
>
>>> +	if (unlikely(pool == NULL || s == NULL))
>> Here, the right check is being done, so at the very least, I would make
>> the last line I commented on the same as this one for code continuity.
> ok
>
> [...]
>
>>> +	if (unlikely(!(pool && chunk)))
>> Please make this check the same as the last line I commented on,
>> especially since it's the same struct being checked.
> yes

OK :-)


>
> [...]
>
>>> +	if (!name) {
>>> +		WARN_ON(1);
>> ??  Maybe the name check should be in WARN_ON()?
> true :-(

OK.


>
> [...]
>
>>> +	if (unlikely(!req_size || !pool))
>> same unlikely() check problem mentioned before.
>>> +		return -1;
>> Can we use an errno value instead for better diagnosibility?
>>> +
>>> +	data = pool->data;
>>> +
>>> +	if (data == NULL)
>>> +		return -1;
>> Same here (ENOMEM or ENXIO comes to mind).
>>> +
>>> +	if (unlikely(data->protected)) {
>>> +		WARN_ON(1);
>> Maybe re-write this with the check inside WARN_ON()?
>>> +		return -1;
>> Same here, how about a different errno value for this case?
> yes, to all of the above

OK.

>
> [...]
>
>>> +static void pmalloc_chunk_set_protection(struct gen_pool *pool,
>>> +
>>> +					 struct gen_pool_chunk *chunk,
>>> +					 void *data)
>>> +{
>>> +	const bool *flag = data;
>>> +	size_t chunk_size = chunk->end_addr + 1 - chunk->start_addr;
>>> +	unsigned long pages = chunk_size / PAGE_SIZE;
>>> +
>>> +	BUG_ON(chunk_size & (PAGE_SIZE - 1));
>> Re-think WARN_ON() for BUG_ON()?  And also check chunk as well, as it's
>> being used below?
> ok
>
>>> +
>>> +	if (*flag)
>>> +		set_memory_ro(chunk->start_addr, pages);
>>> +	else
>>> +		set_memory_rw(chunk->start_addr, pages);
>>> +}
>>> +
>>> +static int pmalloc_pool_set_protection(struct gen_pool *pool, bool protection)
>>> +{
>>> +	struct pmalloc_data *data;
>>> +	struct gen_pool_chunk *chunk;
>>> +
>>> +	if (unlikely(!pool))
>>> +		return -EINVAL;
>> This is example of what I'd perfer seeing in check_alloc_params().
> yes
>
>>> +
>>> +	data = pool->data;
>>> +
>>> +	if (unlikely(!data))
>>> +		return -EINVAL;
>> ENXIO or EIO or ENOMEM sound better?
> Why? At least based on he description from errno-base.h, EINVAL seemed
> the most appropriate:
>
> #define	EIO		 5	/* I/O error */
> #define	ENXIO		 6	/* No such device or address */
> #define	ENOMEM		12	/* Out of memory */
>
> #define	EINVAL		22	/* Invalid argument */
>
> If I was really pressed to change it, I'd rather pick:
>
> #define	EFAULT		14	/* Bad address */
>

OK, very reasonable.


>>> +
>>> +	if (unlikely(data->protected == protection)) {
>>> +		WARN_ON(1);
>> Better to put the check inside WARN_ON, me thinks...
> yes, I have no idea why I wrote it like that  :-(

No worries :-).


>
>>> +		return 0;
>>> +	}
>>> +
>>> +	data->protected = protection;
>>> +	list_for_each_entry(chunk, &(pool)->chunks, next_chunk)
>>> +		pmalloc_chunk_set_protection(pool, chunk, &protection);
>>> +	return 0;
>>> +}
>>> +
>>> +int pmalloc_protect_pool(struct gen_pool *pool)
>>> +{
>>> +	return pmalloc_pool_set_protection(pool, true);
>> Is pool == NULL being checked somewhere, similar to previous functions
>> in this patch?
> right.

OK.


>
>>> +}
>>> +
>>> +
>>> +static void pmalloc_chunk_free(struct gen_pool *pool,
>>> +			       struct gen_pool_chunk *chunk, void *data)
>>> +{
>> Wat is 'data' being used for? Looks unused.  Should parameters be
>> checked, like other ones?
>
> This is the iterator that is passed to genalloc
> genalloc defines the format for the iterator, because it *will* want to
> pass the pointer to the opaque data structure.

OK.


>
>>> +	untag_chunk(chunk);
>>> +	gen_pool_flush_chunk(pool, chunk);
>>> +	vfree_atomic((void *)chunk->start_addr);
>>> +}
>>> +
>>> +
>>> +int pmalloc_destroy_pool(struct gen_pool *pool)
>>> +{
>>> +	struct pmalloc_data *data;
>>> +
>>> +	if (unlikely(pool == NULL))
>>> +		return -EINVAL;
>>> +
>>> +	data = pool->data;
>>> +
>>> +	if (unlikely(data == NULL))
>>> +		return -EINVAL;
>> I'd use a different errno value since you already used it for pool.
> Thinking more about this, how about collapsing them?
> I do need to check for the value of data, before I dereference it,
> however the causes are very different:
>
> * wrong pool pointer - definitely possible
> * corrupted data structure (data member overwritten) - highly unlikely


That sounds good.


>>> +
>>> +	mutex_lock(&pmalloc_mutex);
>>> +	list_del(&data->node);
>>> +	mutex_unlock(&pmalloc_mutex);
>>> +
>>> +	if (likely(data->pool_kobject))
>>> +		pmalloc_disconnect(data, data->pool_kobject);
>>> +
>>> +	pmalloc_pool_set_protection(pool, false);
>>> +	gen_pool_for_each_chunk(pool, pmalloc_chunk_free, NULL);
>>> +	gen_pool_destroy(pool);
>>> +	kfree(data);
>> Does data need to be set to NULL in this case, as data is a member of
>> pool (pool->data)?  I'm worried about dangling pointer scenarios which
>> probably isn't good for security modules?
> The pool was destroyed in the previous step.
> There is nothing left that can be set to NULL.
> Unless we are expecting an use-after-free of the pool structure.
> But everything it was referring to is gone as well.
>
> If we really want to go after this case, then basically everything that
> has been allocated should be poisoned before being freed.
>
> Isn't it a bit too much?

I'd ask one of the maintainers (James or Serge) if it's too much, this 
is the Linux Security Module after all.  But it also could be a kernel 
hardening thing to do?

>
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * When the sysfs is ready to receive registrations, connect all the
>>> + * pools previously created. Also enable further pools to be connected
>>> + * right away.
>>> + */
>>> +static int __init pmalloc_late_init(void)
>>> +{
>>> +	struct pmalloc_data *data, *n;
>>> +
>>> +	pmalloc_kobject = kobject_create_and_add("pmalloc", kernel_kobj);
>>> +
>>> +	mutex_lock(&pmalloc_mutex);
>>> +	pmalloc_list = &pmalloc_final_list;
>>> +
>>> +	if (likely(pmalloc_kobject != NULL)) {
>>> +		list_for_each_entry_safe(data, n, &pmalloc_tmp_list, node) {
>>> +			list_move(&data->node, &pmalloc_final_list);
>>> +			pmalloc_connect(data);
>>> +		}
>>> +	}
>> It would be nice to have the init() return an error value in case of
>> failure.
> ok
>
> --
> igor

Thanks,
Jay

Powered by blists - more mailing lists