From: Wen Yang <[email protected]>
Since eventfd's document has clearly stated: A write(2) call adds
the 8-byte integer value supplied in its buffer to the counter.
However, in the current implementation, the following code snippet
did not cause an error:
char str[16] = "hello world";
uint64_t value;
ssize_t size;
int fd;
fd = eventfd(0, 0);
size = write(fd, &str, strlen(str));
printf("eventfd: test writing a string, size=%ld\n", size);
size = read(fd, &value, sizeof(value));
printf("eventfd: test reading as uint64, size=%ld, valus=0x%lX\n",
size, value);
close(fd);
And its output is:
eventfd: test writing a string, size=8
eventfd: test reading as uint64, size=8, valus=0x6F77206F6C6C6568
By checking whether count is equal to sizeof(ucnt), such errors
could be detected. It also follows the requirements of the manual.
Signed-off-by: Wen Yang <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Eric Biggers <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
fs/eventfd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/eventfd.c b/fs/eventfd.c
index fc4d81090763..9afdb722fa92 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -251,7 +251,7 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c
ssize_t res;
__u64 ucnt;
- if (count < sizeof(ucnt))
+ if (count != sizeof(ucnt))
return -EINVAL;
if (copy_from_user(&ucnt, buf, sizeof(ucnt)))
return -EFAULT;
--
2.25.1
On Wed, Feb 07, 2024 at 12:35:18AM +0800, [email protected] wrote:
> From: Wen Yang <[email protected]>
>
> Since eventfd's document has clearly stated: A write(2) call adds
> the 8-byte integer value supplied in its buffer to the counter.
>
> However, in the current implementation, the following code snippet
> did not cause an error:
>
> char str[16] = "hello world";
> uint64_t value;
> ssize_t size;
> int fd;
>
> fd = eventfd(0, 0);
> size = write(fd, &str, strlen(str));
> printf("eventfd: test writing a string, size=%ld\n", size);
> size = read(fd, &value, sizeof(value));
> printf("eventfd: test reading as uint64, size=%ld, valus=0x%lX\n",
> size, value);
>
> close(fd);
>
> And its output is:
> eventfd: test writing a string, size=8
> eventfd: test reading as uint64, size=8, valus=0x6F77206F6C6C6568
>
> By checking whether count is equal to sizeof(ucnt), such errors
> could be detected. It also follows the requirements of the manual.
>
> Signed-off-by: Wen Yang <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: David Woodhouse <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Eric Biggers <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> ---
Seems sensible but has the potential to break users that rely on this
but then again glibc already enforces a 64bit value via eventfd_write()
and eventfd_read().
On Wed, 07 Feb 2024 00:35:18 +0800, [email protected] wrote:
> Since eventfd's document has clearly stated: A write(2) call adds
> the 8-byte integer value supplied in its buffer to the counter.
>
> However, in the current implementation, the following code snippet
> did not cause an error:
>
> char str[16] = "hello world";
> uint64_t value;
> ssize_t size;
> int fd;
>
> [...]
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/1] eventfd: strictly check the count parameter of eventfd_write to avoid inputting illegal strings
https://git.kernel.org/vfs/vfs/c/325e56e9236e
On Wed, Feb 07, 2024 at 12:35:18AM +0800, [email protected] wrote:
> By checking whether count is equal to sizeof(ucnt), such errors
> could be detected. It also follows the requirements of the manual.
Does it? This is what the eventfd manual page says:
A write(2) fails with the error EINVAL if the size of the supplied buffer
is less than 8 bytes, or if an attempt is made to write the value
0xffffffffffffffff.
So, *technically* it doesn't mention the behavior if the size is greater than 8
bytes. But one might assume that such writes are accepted, since otherwise it
would have been mentioned that they're rejected, just like writes < 8 bytes.
If the validation is indeed going to be made more strict, the manual page will
need to be fixed alongside it.
- Eric
On 2024/2/8 12:33, Eric Biggers wrote:
> On Wed, Feb 07, 2024 at 12:35:18AM +0800, [email protected] wrote:
>> By checking whether count is equal to sizeof(ucnt), such errors
>> could be detected. It also follows the requirements of the manual.
> Does it? This is what the eventfd manual page says:
>
> A write(2) fails with the error EINVAL if the size of the supplied buffer
> is less than 8 bytes, or if an attempt is made to write the value
> 0xffffffffffffffff.
>
> So, *technically* it doesn't mention the behavior if the size is greater than 8
> bytes. But one might assume that such writes are accepted, since otherwise it
> would have been mentioned that they're rejected, just like writes < 8 bytes.
Thank you for your commtents.
Although this behavior was not mentioned, it may indeed lead to
undefined performance, such as (we changed char [] to char *):
#include <sys/eventfd.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
int main()
{
//char str[32] = "hello world";
char *str = "hello world";
uint64_t value;
ssize_t size;
int fd;
fd = eventfd(0, 0);
size = write(fd, &str, strlen(str));
printf("eventfd: test writing a string:%s, size=%ld\n", str, size);
size = read(fd, &value, sizeof(value));
printf("eventfd: test reading as uint64, size=%ld, value=0x%lX\n",
size, value);
close(fd);
return 0;
}
$ ./a.out
eventfd: test writing a string:hello world, size=8
eventfd: test reading as uint64, size=8, value=0x560CC0134008
$ ./a.out
eventfd: test writing a string:hello world, size=8
eventfd: test reading as uint64, size=8, value=0x55A3CD373008
$ ./a.out
eventfd: test writing a string:hello world, size=8
eventfd: test reading as uint64, size=8, value=0x55B8D7B99008
--
Best wishes,
Wen
>
> If the validation is indeed going to be made more strict, the manual page will
> need to be fixed alongside it.
>
> - Eric
On Wed, Feb 07, 2024 at 08:33:54PM -0800, Eric Biggers wrote:
> On Wed, Feb 07, 2024 at 12:35:18AM +0800, [email protected] wrote:
> > By checking whether count is equal to sizeof(ucnt), such errors
> > could be detected. It also follows the requirements of the manual.
>
> Does it? This is what the eventfd manual page says:
>
> A write(2) fails with the error EINVAL if the size of the supplied buffer
> is less than 8 bytes, or if an attempt is made to write the value
> 0xffffffffffffffff.
>
> So, *technically* it doesn't mention the behavior if the size is greater than 8
> bytes. But one might assume that such writes are accepted, since otherwise it
> would have been mentioned that they're rejected, just like writes < 8 bytes.
>
> If the validation is indeed going to be made more strict, the manual page will
> need to be fixed alongside it.
Do you prefer we drop this patch?