[PATCH 1/2] sysctl: read() must consume poll events, not poll()

[PATCH 1/2] sysctl: read() must consume poll events, not poll()

[Jason A. Donenfeld]

Events that poll() responds to are supposed to be consumed when the file
is read(), not by the poll() itself. By putting it on the poll() itself,
it makes it impossible to poll() on a epoll file descriptor, since the
event gets consumed too early. Jann wrote a PoC, available in the link
below.

Reported-by: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/lkml/CAG48ez1F0P7Wnp=PGhiUej=u=8CSF6gpD9J=Oxxg0buFRqV1tA@mail.gmail.com/
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 fs/proc/proc_sysctl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 7d9cfc730bd4..1aa145794207 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -622,6 +622,14 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 
 static ssize_t proc_sys_read(struct kiocb *iocb, struct iov_iter *iter)
 {
+	struct inode *inode = file_inode(iocb->ki_filp);
+	struct ctl_table_header *head = grab_header(inode);
+	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
+
+	if (!IS_ERR(head) && table->poll)
+		iocb->ki_filp->private_data = proc_sys_poll_event(table->poll);
+	sysctl_head_finish(head);
+
 	return proc_sys_call_handler(iocb, iter, 0);
 }
 
@@ -668,10 +676,8 @@ static __poll_t proc_sys_poll(struct file *filp, poll_table *wait)
 	event = (unsigned long)filp->private_data;
 	poll_wait(filp, &table->poll->wait, wait);
 
-	if (event != atomic_read(&table->poll->event)) {
-		filp->private_data = proc_sys_poll_event(table->poll);
+	if (event != atomic_read(&table->poll->event))
 		ret = EPOLLIN | EPOLLRDNORM | EPOLLERR | EPOLLPRI;
-	}
 
 out:
 	sysctl_head_finish(head);
-- 
2.35.1

Re: [PATCH 1/2] sysctl: read() must consume poll events, not poll()

[Lucas De Marchi]

On Mon, May 02, 2022 at 04:06:01PM +0200, Jason A. Donenfeld wrote:
>Events that poll() responds to are supposed to be consumed when the file
>is read(), not by the poll() itself. By putting it on the poll() itself,
>it makes it impossible to poll() on a epoll file descriptor, since the
>event gets consumed too early. Jann wrote a PoC, available in the link
>below.
>
>Reported-by: Jann Horn <jannh@google.com>
>Cc: Kees Cook <keescook@chromium.org>
>Cc: Luis Chamberlain <mcgrof@kernel.org>
>Cc: linux-fsdevel@vger.kernel.org
>Link: https://lore.kernel.org/lkml/CAG48ez1F0P7Wnp=PGhiUej=u=8CSF6gpD9J=Oxxg0buFRqV1tA@mail.gmail.com/
>Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

It seems to be my bug. This is indeed better. Also, I don't think it's unsafe
to fix it like this neither. If my memory serves (it's what, 10+ years?), this
was only tested and used with poll(), which will continue to work.

There were plans to use it in one of systemd's tools, in which case we'd
probably notice the misbehavior with epoll().... humn, checking now systemd's
codebase:

static int on_hostname_change(sd_event_source *es, int fd, uint32_t revents, void *userdata) {
	...
	log_info("System hostname changed to '%s'.", full_hostname);
	...
}

static int manager_watch_hostname(Manager *m) {
         int r;

         assert(m);

         m->hostname_fd = open("/proc/sys/kernel/hostname",
                               O_RDONLY|O_CLOEXEC|O_NONBLOCK|O_NOCTTY);
         if (m->hostname_fd < 0) {
                 log_warning_errno(errno, "Failed to watch hostname: %m");
                 return 0;
         }

         r = sd_event_add_io(m->event, &m->hostname_event_source, m->hostname_fd, 0, on_hostname_change, m);
         if (r < 0) {
                 if (r == -EPERM)
                         /* kernels prior to 3.2 don't support polling this file. Ignore the failure. */
                         m->hostname_fd = safe_close(m->hostname_fd);
                 else
                         return log_error_errno(r, "Failed to add hostname event source: %m");
         }
	....
}

and sd_event library uses epoll. So, it's apparently not working and it doesn't
seem to be their intention to rely on the misbehavior. This makes me think it
even deserves a Cc to stable.

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

>---
> fs/proc/proc_sysctl.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>index 7d9cfc730bd4..1aa145794207 100644
>--- a/fs/proc/proc_sysctl.c
>+++ b/fs/proc/proc_sysctl.c
>@@ -622,6 +622,14 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>
> static ssize_t proc_sys_read(struct kiocb *iocb, struct iov_iter *iter)
> {
>+	struct inode *inode = file_inode(iocb->ki_filp);
>+	struct ctl_table_header *head = grab_header(inode);
>+	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
>+
>+	if (!IS_ERR(head) && table->poll)
>+		iocb->ki_filp->private_data = proc_sys_poll_event(table->poll);
>+	sysctl_head_finish(head);
>+
> 	return proc_sys_call_handler(iocb, iter, 0);
> }
>
>@@ -668,10 +676,8 @@ static __poll_t proc_sys_poll(struct file *filp, poll_table *wait)
> 	event = (unsigned long)filp->private_data;
> 	poll_wait(filp, &table->poll->wait, wait);
>
>-	if (event != atomic_read(&table->poll->event)) {
>-		filp->private_data = proc_sys_poll_event(table->poll);
>+	if (event != atomic_read(&table->poll->event))
> 		ret = EPOLLIN | EPOLLRDNORM | EPOLLERR | EPOLLPRI;
>-	}
>
> out:
> 	sysctl_head_finish(head);
>-- 
>2.35.1
>

Re: [PATCH 1/2] sysctl: read() must consume poll events, not poll()

[Jason A. Donenfeld]

Hi Lucas,

On 5/12/22, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Mon, May 02, 2022 at 04:06:01PM +0200, Jason A. Donenfeld wrote:
>>Events that poll() responds to are supposed to be consumed when the file
>>is read(), not by the poll() itself. By putting it on the poll() itself,
>>it makes it impossible to poll() on a epoll file descriptor, since the
>>event gets consumed too early. Jann wrote a PoC, available in the link
>>below.
>>
>>Reported-by: Jann Horn <jannh@google.com>
>>Cc: Kees Cook <keescook@chromium.org>
>>Cc: Luis Chamberlain <mcgrof@kernel.org>
>>Cc: linux-fsdevel@vger.kernel.org
>>Link:
>> https://lore.kernel.org/lkml/CAG48ez1F0P7Wnp=PGhiUej=u=8CSF6gpD9J=Oxxg0buFRqV1tA@mail.gmail.com/
>>Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> It seems to be my bug. This is indeed better. Also, I don't think it's
> unsafe
> to fix it like this neither. If my memory serves (it's what, 10+ years?),
> this
> was only tested and used with poll(), which will continue to work.

You are not correct. Please read the entire thread. This breaks systemd.

Jason