strnlen_user() currently assumes that it is able to read the entire user string word by word, potentially under- or over-reading.
Unfortunately such assumption is invalid in PCuABI, as the bounds of the input user (capability) pointer may exactly match that of the string, preventing any under- or over-read, even by a few bytes.
do_strnlen_user() is therefore amended to check the bounds (base and limit) of the input pointer. If the bounds do not include the entire first or last word, the head and/or tail characters are read byte by byte (find_zero_unaligned()). The rest of the string is still read using the faster, word-by-word find_zero_aligned().
This change should be a no-op for standard ABIs (retaining the unconditional word-by-word loop), as the bounds of non-capability pointers are considered unlimited.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com ---
This patch turned out to be a lot more complex than I expected, I hope it remains mostly readable. I did some exhaustive testing of all combinations of string bounds and capability bounds for short strings (which revealed many issues along the way); this however requires adding a new syscall directly exposing strnlen_user(), which is straightforward but cannot be merged. There is nothing particularly smart about the test itself, it simply consists in a bunch of nested loops.
lib/strnlen_user.c | 104 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 93 insertions(+), 11 deletions(-)
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c index 60942cdfd3d2..8f12259d8431 100644 --- a/lib/strnlen_user.c +++ b/lib/strnlen_user.c @@ -18,6 +18,9 @@ static __always_inline long find_zero_aligned(const unsigned long __user *src, long res = 0; unsigned long c;
+ if (max == 0) + return 0; + unsafe_get_user(c, src++, efault); c |= aligned_byte_mask(align);
@@ -42,6 +45,27 @@ static __always_inline long find_zero_aligned(const unsigned long __user *src, return -EFAULT; }
+/* + * Returns the *index* of '\0' in src, or max if not found. + */ +static __always_inline long find_zero_unaligned(const char __user *src, + unsigned long max) +{ + long res; + + for (res = 0; res < max; res++) { + char c; + + unsafe_get_user(c, src++, efault); + if (c == '\0') + break; + } + + return res; +efault: + return -EFAULT; +} + /* * Do a strnlen, return length of string *with* final '\0'. * 'count' is the user-supplied count, while 'max' is the @@ -57,23 +81,81 @@ static __always_inline long find_zero_aligned(const unsigned long __user *src, */ static __always_inline long do_strnlen_user(const char __user *src, long count, unsigned long max) { - unsigned long align; - long res; + unsigned long align, tail = 0; + long ret, res = 0; + ptraddr_t src_addr = untagged_addr(src); + ptraddr_t src_base = user_ptr_base(src); + ptraddr_t src_limit = user_ptr_limit(src);
/* - * Do everything aligned. But that means that we - * need to also expand the maximum.. + * First check that the pointer's address is within its bounds. + * If not, uaccess would fail, so return 0. Checking this now + * ensures that further calculations are valid. */ - align = (sizeof(unsigned long) - 1) & user_ptr_addr(src); - src -= align; - max += align; + if (src_base > src_addr || src_limit <= src_addr) + return 0;
- res = find_zero_aligned((unsigned long __user *)src, max, align); + align = (sizeof(unsigned long) - 1) & src_addr;
- if (res < 0) - return 0; + if (src_limit < ALIGN(src_addr + max, sizeof(unsigned long))) { + /* + * We cannot read all the words until src + max. Reduce max + * accordingly and calculate how many tail characters will need + * to be read byte by byte. + */ + max = src_limit - src_addr; + tail = (sizeof(unsigned long) - 1) & (src_addr + max); + } + + if (src_base > src_addr - align || max + align == tail) { + /* + * We cannot read the entire first aligned word, as part of it + * cannot be accessed. + */ + unsigned long head; + + if (max + align == tail) { + /* + * Less than a word can be read (see limit check above) + * - read everything byte by byte. + */ + head = max; + } else { + /* + * Read byte by byte until the next word (or return + * right away if we have already reached max). + */ + head = min(sizeof(unsigned long) - align, max); + } + + ret = find_zero_unaligned(src, head); + res += ret; + if (ret < head || max == head) + goto out; + + align = 0; + src += head; + max -= head; + } else { + /* Read the entire first aligned word, adjust max accordingly. */ + src -= align; + max += align; + }
- res -= align; + max -= tail; + ret = find_zero_aligned((unsigned long __user *)src, max, align); + res += ret - align; + if (ret < max) + goto out; + + if (tail) { + ret = find_zero_unaligned(src + max, tail); + res += ret; + } + +out: + if (ret < 0) + return 0;
/* * find_zero_aligned() may end up reading more than count bytes.