linux-kernel - Re: [RFC 1/1] LSM ptags: Add tagging of processes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 26 Nov 2016 13:25:05 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     jobol@...adev.net
Cc:     linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC 1/1] LSM ptags: Add tagging of processes

Jose Bollo wrote:
> +/**
> + * is_valid_utf8 - Is buffer a valid utf8 string?
> + *
> + * @buffer: the start of the string
> + * @length: length in bytes of the buffer
> + *
> + * Return 1 when valid or else returns 0
> + */

Do we really need to check UTF-8 inside kernel? What do you do if
people start using UTF-32 in the future? There was a discussion
about use of encoding inside kernel started at
http://lkml.kernel.org/r/20071103164303.GA26707@ubuntu .

> +
> +/**
> + * _ptags_read - Implement the reading of the tags
> + *
> + * @ptags: tags structure of the readen task
> + * @result: a pointer for storing the read result
> + * @uns: user namespace proxy
> + *
> + * Returns the count of byte read or the negative code -ENOMEM
> + * if an allocation failed.
> + */
> +static int _ptags_read(struct _ptags *ptags, char **result, struct uns uns)
> +{
> +	unsigned idx, count;
> +	size_t size;
> +	struct entry *entries, *entry;
> +	char *buffer;
> +	struct value *value;
> +	struct item *item;
> +
> +	/* init loops */
> +	count = ptags->count;
> +	entries = ptags->entries;
> +
> +	/* compute printed size */
> +	size = 0;
> +	for (idx = 0; idx < count; idx++) {
> +		entry = &entries[idx];
> +		value = entry_read(entry, uns);
> +		if (value) {
> +			item = value_get(*value);
> +			size += entry_name(*entry)->length
> +				+ (unsigned)value_is_kept(*value)
> +				+ (item ? 2 + item->length : 1);
> +		}
> +	}
> +
> +	if (size > INT_MAX)
> +		return -E2BIG;

This sanity check is useless. INT_MAX is 2,147,483,647 but kmalloc() can't
allocate larger than 8,388,608 bytes if PAGE_SIZE = 4096 and MAX_ORDER = 11.

> +	buffer = kmalloc(size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;

Moreover, kmalloc() will not try to allocate larger than 32,768 bytes
(PAGE_ALLOC_COSTLY_ORDER = 3). Although __GFP_NOFAIL can force kmalloc() to
retry by invoking the OOM killer, such behavior might change in near future
due to http://lkml.kernel.org/r/20161123064925.9716-3-mhocko@kernel.org .

Given these constants

#define MAXCOUNT	4000
#define MAXTAGLEN	4000
#define MAXVALUELEN	32700

and someone tried to use as many and long as possible tags, what is
possible max value for "size"? I think that that value can easily exceed
32,768 and kmalloc() won't be reliable. vmalloc() can be used as a fallback
when kmalloc() failed, but is trying to pass possible max value for "size"
to vmalloc() reasonable? Shouldn't this function be rewritten not to
allocate so much memory?

Also, how much memory will be consumed if everybody tried to use tags
as many and long as possible?

> +
> +	/* print in the buffer */
> +	*result = buffer;
> +	for (idx = 0; idx < count; idx++) {
> +		entry = &entries[idx];
> +		value = entry_read(entry, uns);
> +		if (value) {
> +			if (value_is_kept(*value))
> +				*buffer++ = KEEP_CHAR;
> +			item = entry_name(*entry);
> +			memcpy(buffer, item->value, item->length);
> +			buffer += item->length;
> +			item = value_get(*value);
> +			if (item) {
> +				*buffer++ = ASSIGN_CHAR;
> +				memcpy(buffer, item->value, item->length);
> +				buffer += item->length;
> +			}
> +			*buffer++ = EOL_CHAR;
> +		}
> +	}
> +
> +	return (int)size;
> +}