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:
signature.asc
Description: PGP signature
