linux-kernel - Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)

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:	Thu, 7 Jan 2016 10:36:37 +0800
From:	Minfei Huang <mhuang@...hat.com>
To:	Xunlei Pang <xlpang@...hat.com>
Cc:	kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
	ebiederm@...ssion.com, akpm@...ux-foundation.org,
	Dave Young <dyoung@...hat.com>, Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [PATCH v2 2/2] kexec: Provide
 arch_kexec_protect(unprotect)_crashkres()

On 01/07/16 at 10:14am, Xunlei Pang wrote:
> >> +static int
> >> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
> >> +{
> >> +	struct page *page;
> >> +	unsigned int nr_pages;
> >> +
> >> +	/* For physical range: [start, end] */
> >> +	if (!start || !end || start > end)
> >> +		return 0;
> > Hi, Xunlei.
> >
> >         if (start > end)
> >                 return 0;
> 
> If both start and end are zero, we want to return directly, so the two
> more check doesn't hurt.

How about if the start is equal to 0, and end is larger than 0? It is
better to make code more robust, although it never happen in currect
kexec code.

> 
> > See the below comment.
> >> +
> >> +	page = pfn_to_page(start >> PAGE_SHIFT);
> >> +	nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;
> > As I commented in last version, it is better to cover the case if the
> > range from start to end acrosses two pages.
> 
> right.
> 
> >> +	if (protect)
> >> +		return set_pages_ro(page, nr_pages);
> >> +	else
> >> +		return set_pages_rw(page, nr_pages);
> >> +}
> >> +
> >> +static void kexec_mark_crashkres(bool protect)
> >> +{
> >> +	unsigned long control;
> >> +
> >> +	kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
> > Adding the following if test to test crashk_low_res is better. Then we
> > do not need to test if start or end is equal to 0 in kexec_mark_range.
> >
> >         if (crashk_low_res.start != crashk_low_res.end) {
> >                 kexec_mark_range(crashk_low_res.start,
> >                                 crashk_low_res.end, protect);
> >         }
> 
> The checks in kexec_mark_range() will handle the case, it's not
> performance-critical path and will make the code less clean.
> 
> >> +
> >> +	/* Don't touch the control code page used in crash_kexec().*/
> >> +	control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
> >> +	/* Control code page is located in the 2nd page. */
> >> +	kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
> >> +	kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);
> > I think it is more readable, if we use MACRO KEXEC_CONTROL_PAGE_SIZE,
> > instead of using 2*PAGE_SIZE directly.
> 
> OK.
> 
> How about the following update:
> +static void kexec_mark_crashkres(bool protect)
> +{
> +       unsigned long control;
> +
> +       kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
> +
> +       /* Don't touch the control code page used in crash_kexec().*/
> +       control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
> +       /* Control code page is located in the 2nd page. */
> +       kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
> +       control += KEXEC_CONTROL_PAGE_SIZE;
> +       kexec_mark_range(control, crashk_res.end, protect);
> +}

I'm fine with this.

Thanks
Minfei
--
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/