linux-kernel - Re: [Xen-devel] [PATCH -next] x86/xen: Fix read buffer overflow

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] [day] [month] [year] [list]

Date:   Wed, 19 Dec 2018 00:56:42 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:     Andrew Cooper <andrew.cooper3@...rix.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Juergen Gross <jgross@...e.com>, sstabellini@...nel.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        xen-devel@...ts.xenproject.org, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Xen-devel] [PATCH -next] x86/xen: Fix read buffer overflow

On Tue, Dec 18, 2018 at 12:35:34PM -0500, Boris Ostrovsky wrote:
> On 12/18/18 6:28 AM, Andrew Cooper wrote:
> > On 18/12/2018 10:42, YueHaibing wrote:
> >> On 2018/12/18 16:31, Juergen Gross wrote:
> >>> On 18/12/2018 09:19, YueHaibing wrote:
> >>>> Fix smatch warning:
> >>>>
> >>>> arch/x86/xen/enlighten_pv.c:649 get_trap_addr() error:
> >>>>  buffer overflow 'early_idt_handler_array' 32 <= 32
> >>>>
> >>>> Fixes: 42b3a4cb5609 ("x86/xen: Support early interrupts in xen pv guests")
> >>>> Signed-off-by: YueHaibing <yuehaibing@...wei.com>
> >>>> ---
> >>>>  arch/x86/xen/enlighten_pv.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> >>>> index 2f6787f..81f200d 100644
> >>>> --- a/arch/x86/xen/enlighten_pv.c
> >>>> +++ b/arch/x86/xen/enlighten_pv.c
> >>>> @@ -646,7 +646,7 @@ static bool __ref get_trap_addr(void **addr, unsigned int ist)
> >>>>  
> >>>>  	if (nr == ARRAY_SIZE(trap_array) &&
> >>>>  	    *addr >= (void *)early_idt_handler_array[0] &&
> >>>> -	    *addr < (void *)early_idt_handler_array[NUM_EXCEPTION_VECTORS]) {
> >>>> +	    *addr < (void *)early_idt_handler_array[NUM_EXCEPTION_VECTORS - 1]) {
> >>>>  		nr = (*addr - (void *)early_idt_handler_array[0]) /
> >>>>  		     EARLY_IDT_HANDLER_SIZE;
> >>>>  		*addr = (void *)xen_early_idt_handler_array[nr];
> >>>>
> >>> No, this patch is wrong.
> >>>
> >>> early_idt_handler_array is a 2-dimensional array:
> >>>
> >>> const char
> >>> early_idt_handler_array[NUM_EXCEPTION_VECTORS][EARLY_IDT_HANDLER_SIZE];
> >>>
> >>> So above code doesn't do an out of bounds array access, but checks for
> >>> *addr being in the array or outside of it (note the "<" used for the
> >>> test).
> >> Thank you for your explanation.
> > This looks like a smatch bug.  I'd feed it back upstream.
> 
> +Dan
> 

Yep.  Thanks for the bug report.  Let me test my fix and push it later
this week.

Btw, it might help readability slightly if we made it more clear we were
doing pointer math:

		*addr >= (void *)&early_idt_handler_array[0] &&
		*addr < (void *)&early_idt_handler_array[NUM_EXCEPTION_VECTORS]) {
			nr = (*addr - (void *)&early_idt_handler_array[0]) /

Regardless, this is definitely a bug in Smatch and I will push a fix.

regards,
dan carpenter