* Re: [PATCH -next] x86/xen: Fix read buffer overflow
[not found] <20181218081910.18080-1-yuehaibing@huawei.com>
@ 2018-12-18 8:31 ` Juergen Gross
[not found] ` <7825d772-338a-e39e-eaff-73e666ef5c08@suse.com>
1 sibling, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2018-12-18 8:31 UTC (permalink / raw)
To: YueHaibing, boris.ostrovsky, sstabellini, tglx, mingo, bp, hpa
Cc: xen-devel, x86, linux-kernel
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@huawei.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).
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] x86/xen: Fix read buffer overflow
[not found] ` <7825d772-338a-e39e-eaff-73e666ef5c08@suse.com>
@ 2018-12-18 10:42 ` YueHaibing
[not found] ` <2fe8f6b7-b791-e7ea-6484-491e089321d5@huawei.com>
1 sibling, 0 replies; 6+ messages in thread
From: YueHaibing @ 2018-12-18 10:42 UTC (permalink / raw)
To: Juergen Gross, boris.ostrovsky, sstabellini, tglx, mingo, bp, hpa
Cc: xen-devel, x86, linux-kernel
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@huawei.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.
>
>
> Juergen
>
> .
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] x86/xen: Fix read buffer overflow
[not found] ` <2fe8f6b7-b791-e7ea-6484-491e089321d5@huawei.com>
@ 2018-12-18 11:28 ` Andrew Cooper
[not found] ` <08a359b7-1746-8997-4c19-b60a30ccdd63@citrix.com>
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2018-12-18 11:28 UTC (permalink / raw)
To: YueHaibing, Juergen Gross, boris.ostrovsky, sstabellini, tglx,
mingo, bp, hpa
Cc: xen-devel, x86, linux-kernel
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@huawei.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.
It is explicitly permitted in the C spec to construct a pointer to
one-past-the-end of an array, for the purposes of a < comparison.
I'm not entirely sure where the "32 <= 32" statement is coming from.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] x86/xen: Fix read buffer overflow
[not found] ` <08a359b7-1746-8997-4c19-b60a30ccdd63@citrix.com>
@ 2018-12-18 17:35 ` Boris Ostrovsky
[not found] ` <0de982b7-3402-9321-bd6a-f40de653f6e1@oracle.com>
1 sibling, 0 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2018-12-18 17:35 UTC (permalink / raw)
To: Andrew Cooper, YueHaibing, Juergen Gross, sstabellini, tglx,
mingo, bp, hpa
Cc: xen-devel, x86, linux-kernel, Dan Carpenter
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@huawei.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
>
> It is explicitly permitted in the C spec to construct a pointer to
> one-past-the-end of an array, for the purposes of a < comparison.
>
> I'm not entirely sure where the "32 <= 32" statement is coming from.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] x86/xen: Fix read buffer overflow
[not found] ` <0de982b7-3402-9321-bd6a-f40de653f6e1@oracle.com>
@ 2018-12-18 21:56 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2018-12-18 21:56 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, sstabellini, Andrew Cooper, x86, YueHaibing,
linux-kernel, mingo, bp, hpa, xen-devel, tglx
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@huawei.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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH -next] x86/xen: Fix read buffer overflow
@ 2018-12-18 8:19 YueHaibing
0 siblings, 0 replies; 6+ messages in thread
From: YueHaibing @ 2018-12-18 8:19 UTC (permalink / raw)
To: boris.ostrovsky, jgross, sstabellini, tglx, mingo, bp, hpa
Cc: xen-devel, x86, YueHaibing, linux-kernel
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@huawei.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];
--
2.7.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-18 21:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20181218081910.18080-1-yuehaibing@huawei.com>
2018-12-18 8:31 ` [PATCH -next] x86/xen: Fix read buffer overflow Juergen Gross
[not found] ` <7825d772-338a-e39e-eaff-73e666ef5c08@suse.com>
2018-12-18 10:42 ` YueHaibing
[not found] ` <2fe8f6b7-b791-e7ea-6484-491e089321d5@huawei.com>
2018-12-18 11:28 ` Andrew Cooper
[not found] ` <08a359b7-1746-8997-4c19-b60a30ccdd63@citrix.com>
2018-12-18 17:35 ` Boris Ostrovsky
[not found] ` <0de982b7-3402-9321-bd6a-f40de653f6e1@oracle.com>
2018-12-18 21:56 ` Dan Carpenter
2018-12-18 8:19 YueHaibing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).