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@xxxxxxxxxx> > Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx> > Cc: Jan Kara <jack@xxxxxxx> > Cc: linux-fsdevel@xxxxxxxxxxxxxxx > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> Yeah, good cleanup. Feel free to add: Reviewed-by: Jan Kara <jack@xxxxxxx> 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 > -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
