Re: [PATCH v4] RO/NX protection for loadable kernel modules

[Arjan van de Ven]

On Tue, 7 Jul 2009 20:47:42 -0400
Siarhei Liakh <sliakh.lkml@xxxxxxxxx> wrote:

> 
> The patch have been developed for Linux 2.6.30 by Siarhei Liakh
> <sliakh.lkml@xxxxxxxxx> and Xuxian Jiang <jiang@xxxxxxxxxxx>.
> 
> ---
> 
> Signed-off-by: Siarhei Liakh <sliakh.lkml@xxxxxxxxx>
> Signed-off-by: Xuxian Jiang <jiang@xxxxxxxxxxx>

I like it, you can already put

Acked-by: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx> 

there if you want. If you're going to make a v5 then I do have another
suggestion for improvement... (only possible now that the code is very
clean)

> +	/* Set RO for module text and RO-data*/
> +	if (ro_size > 0) {
> +		begin_addr = (unsigned long) base;
> +		end_addr = begin_addr + ro_size;
> +
> +		/*skip last page if end address is not page-aligned*/
> +		if (!IS_ALIGNED(end_addr, PAGE_SIZE))
> +			end_addr = ALIGN(end_addr - PAGE_SIZE,PAGE_SIZE); 
> +
> +		/*Set text RO if there are still pages between begin
> and end*/
> +		if (end_addr > begin_addr) {
> +			pg_count = PFN_DOWN(end_addr - 1) -
> +					PFN_DOWN(begin_addr) + 1;
> +			DEBUGP("  RO: 0x%lx %lu\n", begin_addr,
> pg_count);
> +			set_memory_ro(begin_addr, pg_count);
> +		} else {
> +			DEBUGP("  RO: less than a page, not
> enforcing.\n");
> +		}
> +	} else {
> +		DEBUGP("  RO: section not present.\n");
> +	}

I *think* this can be done as

begin_pfn = PFN_UP( (base);
end_pfn = PFN_DOWN(base + ro_size);

if (end_pfn > begin_pfn)
	set_memory_ro(begin_pfn >> PAGE_SHIFT, end_pfn - begin_pfn);

(note that I think the +1 you have might be buggy)

if you use PFN_UP/PFN_DOWN like this (rounding the start up, rounding the end down),
then your entire "fix alignment" is not needed, the PFN rounding will automatically
take case of this.

similar construct also applies to the NX codepath that follows right after this RO codepath.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/