linux-kernel - Re: [linux-next PATCH v3] drivers/virt/fsl

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:   Mon, 31 Aug 2020 15:58:19 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Souptick Joarder <jrdr.linux@...il.com>,
        <akpm@...ux-foundation.org>
CC:     <jgg@...pe.ca>, <dan.j.williams@...el.com>,
        <gregkh@...uxfoundation.org>, <arnd@...db.de>,
        <timur@...escale.com>, <galak@...nel.crashing.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [linux-next PATCH v3] drivers/virt/fsl_hypervisor: Fix error
 handling path

On 8/31/20 3:07 PM, Souptick Joarder wrote:
> First, when memory allocation for sg_list_unaligned failed, there
> is a bug of calling put_pages() as we haven't pinned any pages.

"we should unpin"

...
>   
> @@ -250,7 +250,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
>   		num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
>   
>   	if (num_pinned != num_pages) {
> -		/* get_user_pages() failed */
> +		/* get_user_pages_fast() failed */

Let's please just delete that particular comment entirely. It's of
questionable accuracy (partial success is allowed with this API), and it
is echoing the code too closely to be worth the line that it consumes.

More importantly, though, we need to split up the cases of gup_fast
returning a negative value, and a zero or positive value. Either here,
or at "exit:", the negative return case should just skip any attempt to
do any put_page() calls at all. Because it's a maintenance hazard to
leave in a loop that depends on looping from zero, to -ERRNO, and *not*
doing any loops--especially in the signed/unsigned soupy mess around gup
calls.

>   		pr_debug("fsl-hv: could not lock source buffer\n");
>   		ret = (num_pinned < 0) ? num_pinned : -EFAULT;
>   		goto exit;
> @@ -293,12 +293,12 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
>   
>   exit:
>   	if (pages) {
> -		for (i = 0; i < num_pages; i++)
> -			if (pages[i])
> -				put_page(pages[i]);
> +		for (i = 0; i < num_pinned; i++)
> +			put_page(pages[i]);

Looks correct. I sometimes wonder why more callers don't use
release_pages() in situations like this, but that's beyond the scope of
your work here.

thanks,
-- 
John Hubbard
NVIDIA