linux-kernel - Re: [PATCH 1/7] binder: create userspace-to-binder-buffer copy function

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]

Date:   Tue, 29 Jan 2019 11:12:18 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Todd Kjos <tkjos@...roid.com>
Cc:     tkjos@...gle.com, gregkh@...uxfoundation.org, arve@...roid.com,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        maco@...gle.com, joel@...lfernandes.org, kernel-team@...roid.com
Subject: Re: [PATCH 1/7] binder: create userspace-to-binder-buffer copy
 function

On Mon, Jan 28, 2019 at 04:49:28PM -0800, Todd Kjos wrote:
> +/**
> + * binder_alloc_copy_user_to_buffer() - copy src user to tgt user
> + * @alloc: binder_alloc for this proc
> + * @buffer: binder buffer to be accessed
> + * @buffer_offset: offset into @buffer data
> + * @from: userspace pointer to source buffer
> + * @bytes: bytes to copy
> + *
> + * Copy bytes from source userspace to target buffer.
> + *
> + * Return: bytes remaining to be copied
> + */
> +unsigned long
> +binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
> +				 struct binder_buffer *buffer,
> +				 binder_size_t buffer_offset,
> +				 const void __user *from,
> +				 size_t bytes)
> +{
> +	if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> +		return bytes;
> +
> +	while (bytes) {
> +		unsigned long size;
> +		unsigned long ret;
> +		struct page *page;
> +		pgoff_t pgoff;
> +		void *kptr;
> +
> +		page = binder_alloc_get_page(alloc, buffer,
> +					     buffer_offset, &pgoff);
> +		size = min(bytes, (size_t)(PAGE_SIZE - pgoff));

This code has so much more casting than necessary.  To me casting says
that we haven't got the types correct or something.  I've just pulled
this function out as an example really, but none of the casts here are
required...  This could just be:

		size = min(bytes, PAGE_SIZE - pgoff);

Btw, if you really need to do a cast inside a min() (you don't in this
cast) then use min_t().

> +		kptr = (void *)((uintptr_t)kmap(page) + pgoff);

This would be a lot cleaner as:

		kptr = kmap(page) + pgoff;

> +		ret = copy_from_user(kptr, (const void __user *)(uintptr_t)from,
> +				     size);

Remove the cast:

		ret = copy_from_user(kptr, from, size);


> +		kunmap(page);
> +		if (ret)
> +			return bytes - size + ret;
> +		bytes -= size;
> +		from = (void __user *)(uintptr_t)from + size;

Remove the cast:

		from += size;

> +		buffer_offset += size;
> +	}
> +	return 0;
> +}

regards,
dan carpenter