| Message ID | 20240129184014.work.593-kees@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | select: Avoid wrap-around instrumentation in do_sys_poll() | expand |
On Mon, 29 Jan 2024 10:40:15 -0800, Kees Cook wrote: > The mix of int, unsigned int, and unsigned long used by struct > poll_list::len, todo, len, and j meant that the signed overflow > sanitizer got worried it needed to instrument several places where > arithmetic happens between these variables. Since all of the variables > are always positive and bounded by unsigned int, use a single type in > all places. Additionally expand the zero-test into an explicit range > check before updating "todo". > > [...] 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] select: Avoid wrap-around instrumentation in do_sys_poll() https://git.kernel.org/vfs/vfs/c/ecd1f34f1242
On Mon 29-01-24 10:40:15, Kees Cook wrote: > The mix of int, unsigned int, and unsigned long used by struct > poll_list::len, todo, len, and j meant that the signed overflow > sanitizer got worried it needed to instrument several places where > arithmetic happens between these variables. Since all of the variables > are always positive and bounded by unsigned int, use a single type in > all places. Additionally expand the zero-test into an explicit range > check before updating "todo". > > This keeps sanitizer instrumentation[1] out of a UACCESS path: > > vmlinux.o: warning: objtool: do_sys_poll+0x285: call to __ubsan_handle_sub_overflow() with UACCESS enabled > > Link: https://github.com/KSPP/linux/issues/26 [1] > Cc: Christian Brauner <brauner@kernel.org> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Jan Kara <jack@suse.cz> > Cc: linux-fsdevel@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Yeah, good cleanup. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/select.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/select.c b/fs/select.c > index 0ee55af1a55c..11a3b1312abe 100644 > --- a/fs/select.c > +++ b/fs/select.c > @@ -839,7 +839,7 @@ SYSCALL_DEFINE1(old_select, struct sel_arg_struct __user *, arg) > > struct poll_list { > struct poll_list *next; > - int len; > + unsigned int len; > struct pollfd entries[]; > }; > > @@ -975,14 +975,15 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds, > struct timespec64 *end_time) > { > struct poll_wqueues table; > - int err = -EFAULT, fdcount, len; > + int err = -EFAULT, fdcount; > /* Allocate small arguments on the stack to save memory and be > faster - use long to make sure the buffer is aligned properly > on 64 bit archs to avoid unaligned access */ > long stack_pps[POLL_STACK_ALLOC/sizeof(long)]; > struct poll_list *const head = (struct poll_list *)stack_pps; > struct poll_list *walk = head; > - unsigned long todo = nfds; > + unsigned int todo = nfds; > + unsigned int len; > > if (nfds > rlimit(RLIMIT_NOFILE)) > return -EINVAL; > @@ -998,9 +999,9 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds, > sizeof(struct pollfd) * walk->len)) > goto out_fds; > > - todo -= walk->len; > - if (!todo) > + if (walk->len >= todo) > break; > + todo -= walk->len; > > len = min(todo, POLLFD_PER_PAGE); > walk = walk->next = kmalloc(struct_size(walk, entries, len), > @@ -1020,7 +1021,7 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds, > > for (walk = head; walk; walk = walk->next) { > struct pollfd *fds = walk->entries; > - int j; > + unsigned int j; > > for (j = walk->len; j; fds++, ufds++, j--) > unsafe_put_user(fds->revents, &ufds->revents, Efault); > -- > 2.34.1 >
diff --git a/fs/select.c b/fs/select.c index 0ee55af1a55c..11a3b1312abe 100644 --- a/fs/select.c +++ b/fs/select.c @@ -839,7 +839,7 @@ SYSCALL_DEFINE1(old_select, struct sel_arg_struct __user *, arg) struct poll_list { struct poll_list *next; - int len; + unsigned int len; struct pollfd entries[]; }; @@ -975,14 +975,15 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds, struct timespec64 *end_time) { struct poll_wqueues table; - int err = -EFAULT, fdcount, len; + int err = -EFAULT, fdcount; /* Allocate small arguments on the stack to save memory and be faster - use long to make sure the buffer is aligned properly on 64 bit archs to avoid unaligned access */ long stack_pps[POLL_STACK_ALLOC/sizeof(long)]; struct poll_list *const head = (struct poll_list *)stack_pps; struct poll_list *walk = head; - unsigned long todo = nfds; + unsigned int todo = nfds; + unsigned int len; if (nfds > rlimit(RLIMIT_NOFILE)) return -EINVAL; @@ -998,9 +999,9 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds, sizeof(struct pollfd) * walk->len)) goto out_fds; - todo -= walk->len; - if (!todo) + if (walk->len >= todo) break; + todo -= walk->len; len = min(todo, POLLFD_PER_PAGE); walk = walk->next = kmalloc(struct_size(walk, entries, len), @@ -1020,7 +1021,7 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds, for (walk = head; walk; walk = walk->next) { struct pollfd *fds = walk->entries; - int j; + unsigned int j; for (j = walk->len; j; fds++, ufds++, j--) unsafe_put_user(fds->revents, &ufds->revents, Efault);
The mix of int, unsigned int, and unsigned long used by struct poll_list::len, todo, len, and j meant that the signed overflow sanitizer got worried it needed to instrument several places where arithmetic happens between these variables. Since all of the variables are always positive and bounded by unsigned int, use a single type in all places. Additionally expand the zero-test into an explicit range check before updating "todo". This keeps sanitizer instrumentation[1] out of a UACCESS path: vmlinux.o: warning: objtool: do_sys_poll+0x285: call to __ubsan_handle_sub_overflow() with UACCESS enabled Link: https://github.com/KSPP/linux/issues/26 [1] Cc: Christian Brauner <brauner@kernel.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Jan Kara <jack@suse.cz> Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/select.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)