[CRIU] [PATCH] restore: Use in_vma_area helper in restore_priv_vma_content

[Pavel Emelyanov]

On 03/25/2013 04:18 PM, Cyrill Gorcunov wrote:
> On Mon, Mar 25, 2013 at 04:09:48PM +0400, Cyrill Gorcunov wrote:
>> On Mon, Mar 25, 2013 at 04:02:28PM +0400, Andrew Vagin wrote:
>>>> @@ -301,7 +301,7 @@ static int restore_priv_vma_content(pid_t pid)
>>>>  			unsigned char buf[PAGE_SIZE];
>>>>  			void *p;
>>>>  
>>>> -			while (va >= vma->vma.end) {
>>>> +			while (!(in_vma_area(vma, va))) {
>>>
>>> This is correct, but I don't like it. The vma list should be sorted, so
>>> the origin сondition is correct. We need to add a check, that va is in
>>> vma, right after this loop.
>>
>> OK, letme check if I manage to come with some better patch.
> 
> Something like this one, I guess.
> ---
> From: Cyrill Gorcunov <gorcunov at openvz.org>
> Date: Mon, 25 Mar 2013 16:17:05 +0400
> Subject: [PATCH] restore: Validate page address in restore_priv_vma_content
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  cr-restore.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/cr-restore.c b/cr-restore.c
> index 13273c1..f64681a 100644
> --- a/cr-restore.c
> +++ b/cr-restore.c
> @@ -301,12 +301,29 @@ static int restore_priv_vma_content(pid_t pid)
>  			unsigned char buf[PAGE_SIZE];
>  			void *p;
>  
> +			/*
> +			 * The lookup os over *all* possible VMAs
> +			 * read from image file.
> +			 */
>  			while (va >= vma->vma.end) {
>  				if (vma->list.next == &rst_vmas.h)
>  					goto err_addr;
>  				vma = list_entry(vma->list.next, struct vma_area, list);
>  			}
>  
> +			/*
> +			 * Make sure the page address is inside existing VMA
> +			 * and the VMA it refers to still private one, since
> +			 * there is no guarantee that the data from pagemap is
> +			 * valid.
> +			 */
> +			if (va < vma->vma.start) {

Excessive braces.
Plus, there the same if () before the while (). It's worth merging that one here.

> +				goto err_addr;
> +			} else if (unlikely(!vma_priv(&vma->vma))) {
> +				pr_err("Trying to restore page for non-private VMA\n");
> +				goto err_addr;
> +			}
> +
>  			off = (va - vma->vma.start) / PAGE_SIZE;
>  			va += PAGE_SIZE;
>  
>