diff mbox series

eventfd: strictly check the count parameter of eventfd_write to avoid inputting illegal strings

Message ID tencent_10AAA44731FFFA493F9F5501521F07DD4D0A@qq.com
State New
Headers
Series eventfd: strictly check the count parameter of eventfd_write to avoid inputting illegal strings |

Commit Message

Wen Yang Feb. 6, 2024, 4:35 p.m. UTC 
  From: Wen Yang <wenyang.linux@foxmail.com>

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 <wenyang.linux@foxmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Eric Biggers <ebiggers@google.com>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
---
 fs/eventfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christian Brauner Feb. 7, 2024, 9:56 a.m. UTC | #1 
On Wed, 07 Feb 2024 00:35:18 +0800, wenyang.linux@foxmail.com 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
Christian Brauner Feb. 7, 2024, 9:58 a.m. UTC | #2 
On Wed, Feb 07, 2024 at 12:35:18AM +0800, wenyang.linux@foxmail.com wrote:
> From: Wen Yang <wenyang.linux@foxmail.com>
> 
> 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 <wenyang.linux@foxmail.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Eric Biggers <ebiggers@google.com>
> Cc: <linux-fsdevel@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> ---

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().
Eric Biggers Feb. 8, 2024, 4:33 a.m. UTC | #3 
On Wed, Feb 07, 2024 at 12:35:18AM +0800, wenyang.linux@foxmail.com 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
Wen Yang Feb. 8, 2024, 5:34 a.m. UTC | #4 
On 2024/2/8 12:33, Eric Biggers wrote:
> On Wed, Feb 07, 2024 at 12:35:18AM +0800, wenyang.linux@foxmail.com 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
Christian Brauner Feb. 9, 2024, 10:18 a.m. UTC | #5 
On Wed, Feb 07, 2024 at 08:33:54PM -0800, Eric Biggers wrote:
> On Wed, Feb 07, 2024 at 12:35:18AM +0800, wenyang.linux@foxmail.com 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?
diff mbox series

Patch

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;