On Mon 22-01-24 16:26:44, 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
>
> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
Looks good. 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
