linux-kernel - Re: [PATCH 03/13] kexec: Move segment verification code in a separate 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20140604093254.GB4105@pd.tnic>
Date:	Wed, 4 Jun 2014 11:32:55 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
	ebiederm@...ssion.com, hpa@...or.com, mjg59@...f.ucam.org,
	greg@...ah.com, jkosina@...e.cz, dyoung@...hat.com,
	chaowang@...hat.com, bhe@...hat.com, akpm@...ux-foundation.org
Subject: Re: [PATCH 03/13] kexec: Move segment verification code in a
 separate function

On Tue, Jun 03, 2014 at 09:06:52AM -0400, Vivek Goyal wrote:
> Previously do_kimage_alloc() will allocate a kimage structure, copy
> segment list from user space and then do the segment list sanity verification.
> 
> Break down this function in 3 parts. do_kimage_alloc_init() to do actual
> allocation and basic initialization of kimage structure.
> copy_user_segment_list() to copy segment list from user space and
> sanity_check_segment_list() to verify the sanity of segment list as passed
> by user space.
> 
> In later patches, I need to only allocate kimage and not copy segment
> list from user space. So breaking down in smaller functions enables
> re-use of code at other places.

I haven't seen what's going on further in the patchset but from looking at
kimage_normal_alloc() and kimage_crash_alloc()'s guts, they look very
similar and could probably share a common __kimage_alloc which does
do_kimage_alloc_init, copy_user_segment_list, sanity_check_segment_list
and kimage_alloc_control_pages...

One probably would have to actually write it down to see whether it
makes sense though and is not too ugly :-)

> Signed-off-by: Vivek Goyal <vgoyal@...hat.com>
> ---

In any case, it looks ok, just two nitpicks below:

Acked-by: Borislav Petkov <bp@...e.de>

>  kernel/kexec.c | 182 ++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 101 insertions(+), 81 deletions(-)

...

> +static struct kimage *do_kimage_alloc_init(void)
> +{
> +	struct kimage *image;
> +
> +	/* Allocate a controlling structure */
> +	image = kzalloc(sizeof(*image), GFP_KERNEL);
> +	if (!image)
> +		return NULL;
> +
> +	image->head = 0;
> +	image->entry = &image->head;
> +	image->last_entry = &image->head;
> +	image->control_page = ~0; /* By default this does not apply */
> +	image->type = KEXEC_TYPE_DEFAULT;
> +
> +	/* Initialize the list of control pages */
> +	INIT_LIST_HEAD(&image->control_pages);
> +
> +	/* Initialize the list of destination pages */
> +	INIT_LIST_HEAD(&image->dest_pages);
> +
> +	/* Initialize the list of unusable pages */
> +	INIT_LIST_HEAD(&image->unuseable_pages);

If the "e" in "unuseable" bugs you too, like me, you could add this one
to your patchset :-)

http://lkml.kernel.org/r/1392819695-24116-1-git-send-email-bp@alien8.de

...

> @@ -258,22 +292,23 @@ static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  					   get_order(KEXEC_CONTROL_PAGE_SIZE));
>  	if (!image->control_code_page) {
>  		printk(KERN_ERR "Could not allocate control_code_buffer\n");
> -		goto out_free;
> +		goto out_free_image;
>  	}
>  
>  	image->swap_page = kimage_alloc_control_pages(image, 0);
>  	if (!image->swap_page) {
>  		printk(KERN_ERR "Could not allocate swap buffer\n");
> -		goto out_free;
> +		goto out_free_control_pages;
>  	}
>  
>  	*rimage = image;
>  	return 0;
>  
> -out_free:
> +

Superfluous newline.

> +out_free_control_pages:
>  	kimage_free_page_list(&image->control_pages);
> +out_free_image:
>  	kfree(image);
> -out:
>  	return result;
>  }
>

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/