linux-kernel - Re: [PATCH 09/82] select: Avoid wrap-around instrumentation in do_sys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240123180008.sg77hnu5r7nqrgjy@quack3>
Date: Tue, 23 Jan 2024 19:00:08 +0100
From: Jan Kara <jack@...e.cz>
To: Kees Cook <keescook@...omium.org>
Cc: linux-hardening@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	linux-fsdevel@...r.kernel.org,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/82] select: Avoid wrap-around instrumentation in
 do_sys_poll()

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@...iv.linux.org.uk>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: Jan Kara <jack@...e.cz>
> Cc: linux-fsdevel@...r.kernel.org
> Signed-off-by: Kees Cook <keescook@...omium.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.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@...e.com>
SUSE Labs, CR