[linux-morello] Re: [PATCH 03/12] lib: Make strnlen_user() PCuABI-friendly

Hi,

On 10/26/23 14:14, Kevin Brodsky wrote:

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;

I suppose instead of returning negative(-EFAULT) error, returning 0 might be sufficient here. This will make the code slightly simpler as the error value is again later used for not so useful calculation.

} +/*

• • 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;

Same as above.

+}

• /*
  • 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;
  

May be:

if (!ret) return 0;

Thanks, Amit

/* * find_zero_aligned() may end up reading more than count bytes.