Re: [PATCH 4/4] module: clean up RO/NX handling.

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Mon, Nov 09, 2015 at 02:53:57PM +1030, Rusty Russell wrote:

> @@ -1858,74 +1849,75 @@ static void mod_sysfs_teardown(struct module *mod)
>  /*
>   * LKM RO/NX protection: protect module's text/ro-data
>   * from modification and any data from execution.
> + *
> + * General layout of module is:
> + *          [text] [read-only-data] [writable data]
> + * text_size -----^                ^               ^
> + * ro_size ------------------------|               |
> + * size -------------------------------------------|
> + *
> + * These values are always page-aligned (as is base)
>   */
> -void set_page_attributes(void *start, void *end, int (*set)(unsigned long start, int num_pages))
> +static void frob_text(const struct module_layout *layout,
> +		      int (*set_memory)(unsigned long start, int num_pages))
>  {
> -	unsigned long begin_pfn = PFN_DOWN((unsigned long)start);
> -	unsigned long end_pfn = PFN_DOWN((unsigned long)end);
> -
> -	if (end_pfn > begin_pfn)
> -		set(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base,
> +		   layout->text_size >> PAGE_SHIFT);

Should the set_memory() call be skipped if text_size is 0?

>  }
>  
> -static void set_section_ro_nx(void *base,
> -			unsigned long text_size,
> -			unsigned long ro_size,
> -			unsigned long total_size,
> -			int (*set_ro)(unsigned long start, int num_pages),
> -			int (*set_nx)(unsigned long start, int num_pages))
> +static void frob_rodata(const struct module_layout *layout,
> +			int (*set_memory)(unsigned long start, int num_pages))
>  {
> -	/* begin and end PFNs of the current subsection */
> -	unsigned long begin_pfn;
> -	unsigned long end_pfn;
> -
> -	/*
> -	 * Set RO for module text and RO-data:
> -	 * - Always protect first page.
> -	 * - Do not protect last partial page.
> -	 */
> -	if (ro_size > 0)
> -		set_page_attributes(base, base + ro_size, set_ro);
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base + layout->text_size,
> +		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
> +}

Same here, what if ro_size == text_size (no rodata)?

>  
> -	/*
> -	 * Set NX permissions for module data:
> -	 * - Do not protect first partial page.
> -	 * - Always protect last page.
> -	 */
> -	if (total_size > text_size) {
> -		begin_pfn = PFN_UP((unsigned long)base + text_size);
> -		end_pfn = PFN_UP((unsigned long)base + total_size);
> -		if (end_pfn > begin_pfn)
> -			set_nx(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
> -	}
> +static void frob_writable_data(const struct module_layout *layout,
> +			       int (*set_memory)(unsigned long start, int num_pages))
> +{
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base + layout->ro_size,
> +		   (layout->size - layout->ro_size) >> PAGE_SHIFT);
>  }

Ditto for size == ro_size (no writable data).


-- 
Josh