| Message ID | tencent_10AAA44731FFFA493F9F5501521F07DD4D0A@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | eventfd: strictly check the count parameter of eventfd_write to avoid inputting illegal strings | expand |
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
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().
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
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
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 --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;