Undefined Behavior in rw_verify_area() (was: sendfile(2) erroneously yields EINVAL on too large counts)

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3932 bytes --]

Hi Al,

On Mon, Mar 04, 2024 at 02:52:46PM +0100, Alejandro Colomar wrote:
> (By inspecting the kernel code I'm not sure if it avoids UB; I think it
> might be triggering UB; let me debug that and come with an update.)

It does indeed invoke Undefined Behavior, in some platforms: in those
where 'loff_t' is wider than 'size_t'.

To find this, I applied the following change to the kernel, to make sure
that the program below triggers exactly that error:

	alx@debian:~/src/linux/linux/ub$ git diff
	diff --git a/fs/read_write.c b/fs/read_write.c
	index d4c036e82b6c..0cbc64829143 100644
	--- a/fs/read_write.c
	+++ b/fs/read_write.c
	@@ -370,7 +370,7 @@ int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t
					return -EOVERFLOW;
			} else if (unlikely((loff_t) (pos + count) < 0)) {
				if (!unsigned_offsets(file))
	-                               return -EINVAL;
	+                               return -EXFULL;
			}
		}
	 


And to reproduce it, I used Jan's program:

	alx@debian:~/tmp$ uname -r
	6.8.0-rc7-alx-dirty
	alx@debian:~/tmp$ cat sf0.c 
	#define _GNU_SOURCE 1
	#include <errno.h>
	#include <fcntl.h>
	#include <limits.h>
	#include <stdio.h>
	#include <string.h>
	#include <unistd.h>
	#include <sys/sendfile.h>

	int main(void)
	{
		int src = open(".", O_RDWR | O_TMPFILE, 0666);
		write(src, "1234", 4);
		int dst = open(".", O_RDWR | O_TMPFILE, 0666);
		write(src, "1234", 4);
		ssize_t ret = sendfile(dst, src, NULL, SSIZE_MAX);
		printf("%ld\n", (long)ret);
		if (ret < 0)
			printf("%s\n", strerror(errno));
		return 0;
	}

	alx@debian:~/tmp$ cc -Wall -Wextra sf0.c 
	alx@debian:~/tmp$ ./a.out 
	-1
	Exchange full

(BTW, Jan, you can use 'int main(void)' if you're not going to use argv.
ISO C allows it: <http://port70.net/~nsz/c/c11/n1570.html#5.1.2.2.1>.)

Here's the code invoking UB:

	alx@debian:~/src/linux/linux/ub$ find fs/ -type f \
				| grep '\.c$' \
				| xargs grepc -tfd rw_verify_area;
	fs/read_write.c:int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count)
	{
		int mask = read_write == READ ? MAY_READ : MAY_WRITE;
		int ret;

		if (unlikely((ssize_t) count < 0))
			return -EINVAL;

		if (ppos) {
			loff_t pos = *ppos;

			if (unlikely(pos < 0)) {
				if (!unsigned_offsets(file))
					return -EINVAL;
				if (count >= -pos) /* both values are in 0..LLONG_MAX */
					return -EOVERFLOW;
			} else if (unlikely((loff_t) (pos + count) < 0)) {
				if (!unsigned_offsets(file))
					return -EXFULL;
			}
		}

		ret = security_file_permission(file, mask);
		if (ret)
			return ret;

		return fsnotify_file_area_perm(file, mask, ppos, count);
	}


See that -EXFULL (originally it was -EINVAL; I modified it for
debugging).  'count' is positive, thanks to the first check.  'pos' is
also positive, since we're in the 'else' of 'pos < 0'.  So, let's
analyze the following line of code:

	if (unlikely((loff_t) (pos + count) < 0)) {

'pos' is of type 'loff_t', a signed type.
'count' is of type 'size_t', an unsigned type.

Depending on the width of those types, the sum may be performed as
'loff_t' if `sizeof(loff_t) > sizeof(size_t)`, or as 'size_t' if
`sizeof(loff_t) <= sizeof(size_t)`.  Since 'loff_t' is a 64-bit type,
but 'size_t' can be either 32-bit or 64-bit, the former is possible.

In those platforms in which loff_t is wider, the addends are promoted to
'loff_t' before the sum.  And a sum of positive signed values can never
be negative.  If the sum overflows (and the program above triggers
such an overflow), the behavior is undefined.

I suggest the following test:

	if (unlikely(pos > type_max(loff_t) - count)) {

What do you think?  If you agree, I'll send a patch.

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]