Received-SPF: pass (google.com: domain of linux-kernel+bounces-35832-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 23 Jan 2024 19:00:08 +0100
From: Jan Kara <jack@suse.cz>
To: Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
	linux-fsdevel@vger.kernel.org,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/82] select: Avoid wrap-around instrumentation in
 do_sys_poll()
Message-ID: <20240123180008.sg77hnu5r7nqrgjy@quack3>
References: <20240122235208.work.748-kees@kernel.org>
 <20240123002814.1396804-9-keescook@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240123002814.1396804-9-keescook@chromium.org>

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@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Looks good. 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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR