[PATCH] kvm-s390: fix potential array overrun in intercept handling

[PATCH] kvm-s390: fix potential array overrun in intercept handling

[Christian Borntraeger]

Avi, Marcelo,

kvm_handle_sie_intercept uses a jump table to get the intercept handler
for a SIE intercept. Static code analysis revealed a potential problem:
the intercept_funcs jump table was defined to contain (0x48 >> 2) entries,
but we only checked for code > 0x48 which would cause an off-by-one
array overflow if code == 0x48.

Since the table is only populated up to (0x28 >> 2), we can reduce the
jump table size while fixing the off-by-one.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
(patch was refreshed with -U8 to see the full jump table.)
 arch/s390/kvm/intercept.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/s390/kvm/intercept.c
===================================================================
--- linux-2.6.orig/arch/s390/kvm/intercept.c
+++ linux-2.6/arch/s390/kvm/intercept.c
@@ -208,32 +208,32 @@ static int handle_instruction_and_prog(s
 
 	if (rc == -ENOTSUPP)
 		vcpu->arch.sie_block->icptcode = 0x04;
 	if (rc)
 		return rc;
 	return rc2;
 }
 
-static const intercept_handler_t intercept_funcs[0x48 >> 2] = {
+static const intercept_handler_t intercept_funcs[(0x28 >> 2) + 1] = {
 	[0x00 >> 2] = handle_noop,
 	[0x04 >> 2] = handle_instruction,
 	[0x08 >> 2] = handle_prog,
 	[0x0C >> 2] = handle_instruction_and_prog,
 	[0x10 >> 2] = handle_noop,
 	[0x14 >> 2] = handle_noop,
 	[0x1C >> 2] = kvm_s390_handle_wait,
 	[0x20 >> 2] = handle_validity,
 	[0x28 >> 2] = handle_stop,
 };
 
 int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
 {
 	intercept_handler_t func;
 	u8 code = vcpu->arch.sie_block->icptcode;
 
-	if (code & 3 || code > 0x48)
+	if (code & 3 || code > 0x28)
 		return -ENOTSUPP;
 	func = intercept_funcs[code >> 2];
 	if (func)
 		return func(vcpu);
 	return -ENOTSUPP;
 }

Re: [PATCH] kvm-s390: fix potential array overrun in intercept handling

[Avi Kivity]

On 01/21/2010 12:56 PM, Christian Borntraeger wrote:
> Avi, Marcelo,
>
> kvm_handle_sie_intercept uses a jump table to get the intercept handler
> for a SIE intercept. Static code analysis revealed a potential problem:
> the intercept_funcs jump table was defined to contain (0x48>>  2) entries,
> but we only checked for code>  0x48 which would cause an off-by-one
> array overflow if code == 0x48.
>
> Since the table is only populated up to (0x28>>  2), we can reduce the
> jump table size while fixing the off-by-one.
>
>    

>
> -static const intercept_handler_t intercept_funcs[0x48>>  2] = {
> +static const intercept_handler_t intercept_funcs[(0x28>>  2) + 1] = {
>   	[0x00>>  2] = handle_noop,
>   	[0x04>>  2] = handle_instruction,
>   	[0x08>>  2] = handle_prog,
>   	[0x0C>>  2] = handle_instruction_and_prog,
>   	[0x10>>  2] = handle_noop,
>   	[0x14>>  2] = handle_noop,
>   	[0x1C>>  2] = kvm_s390_handle_wait,
>   	[0x20>>  2] = handle_validity,
>   	[0x28>>  2] = handle_stop,
>   };
>    

You can define the array without a size to let the compiler figure out 
the minimum size.

>
>   int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
>   {
>   	intercept_handler_t func;
>   	u8 code = vcpu->arch.sie_block->icptcode;
>
> -	if (code&  3 || code>  0x48)
> +	if (code&  3 || code>  0x28)
>   		return -ENOTSUPP;
>    

And here, check against ARRAY_SIZE() instead of a magic number.


-- 
error compiling committee.c: too many arguments to function

[PATCHv2] kvm-s390: fix potential array overrun in intercept handling

[Christian Borntraeger]

v2: apply Avis suggestions about ARRAY_SIZE.

kvm_handle_sie_intercept uses a jump table to get the intercept handler
for a SIE intercept. Static code analysis revealed a potential problem:
the intercept_funcs jump table was defined to contain (0x48 >> 2) entries,
but we only checked for code > 0x48 which would cause an off-by-one
array overflow if code == 0x48.

Use the compiler and ARRAY_SIZE to automatically set the limits.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

---
(patch was refreshed with -U8 to see the full jump table.)
 arch/s390/kvm/intercept.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/s390/kvm/intercept.c
===================================================================
--- linux-2.6.orig/arch/s390/kvm/intercept.c
+++ linux-2.6/arch/s390/kvm/intercept.c
@@ -208,32 +208,32 @@ static int handle_instruction_and_prog(s
 
 	if (rc == -ENOTSUPP)
 		vcpu->arch.sie_block->icptcode = 0x04;
 	if (rc)
 		return rc;
 	return rc2;
 }
 
-static const intercept_handler_t intercept_funcs[0x48 >> 2] = {
+static const intercept_handler_t intercept_funcs[] = {
 	[0x00 >> 2] = handle_noop,
 	[0x04 >> 2] = handle_instruction,
 	[0x08 >> 2] = handle_prog,
 	[0x0C >> 2] = handle_instruction_and_prog,
 	[0x10 >> 2] = handle_noop,
 	[0x14 >> 2] = handle_noop,
 	[0x1C >> 2] = kvm_s390_handle_wait,
 	[0x20 >> 2] = handle_validity,
 	[0x28 >> 2] = handle_stop,
 };
 
 int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)
 {
 	intercept_handler_t func;
 	u8 code = vcpu->arch.sie_block->icptcode;
 
-	if (code & 3 || code > 0x48)
+	if (code & 3 || (code >> 2)  >= ARRAY_SIZE(intercept_funcs))
 		return -ENOTSUPP;
 	func = intercept_funcs[code >> 2];
 	if (func)
 		return func(vcpu);
 	return -ENOTSUPP;
 }

Re: [PATCHv2] kvm-s390: fix potential array overrun in intercept handling

[Heiko Carstens]

> -	if (code & 3 || code > 0x48)
> +	if (code & 3 || (code >> 2)  >= ARRAY_SIZE(intercept_funcs))
>  		return -ENOTSUPP;

Not that it matters for this patch, but -ENOTSUPP should not leak to
userspace. Not sure if it does somewhere, but it is used all over the
place within arch/s390/kvm...
Use -EOPNOTSUPP or something similar instead.

Re: [PATCHv2] kvm-s390: fix potential array overrun in intercept handling

[Christian Borntraeger]

Am Donnerstag 21 Januar 2010 12:24:18 schrieb Heiko Carstens:
> > -	if (code & 3 || code > 0x48)
> > +	if (code & 3 || (code >> 2)  >= ARRAY_SIZE(intercept_funcs))
> >  		return -ENOTSUPP;
> 
> Not that it matters for this patch, but -ENOTSUPP should not leak to
> userspace. Not sure if it does somewhere, but it is used all over the
> place within arch/s390/kvm...
> Use -EOPNOTSUPP or something similar instead.

AFAICS it does not leak to userspace, ENOTSUPP is an internal code. see
kvm_arch_vcpu_ioctl_run:
[...]
        if (rc == -ENOTSUPP) {
                /* intercept cannot be handled in-kernel, prepare kvm-run */
                kvm_run->exit_reason         = KVM_EXIT_S390_SIEIC;
                kvm_run->s390_sieic.icptcode = vcpu->arch.sie_block->icptcode;
                kvm_run->s390_sieic.ipa      = vcpu->arch.sie_block->ipa;
                kvm_run->s390_sieic.ipb      = vcpu->arch.sie_block->ipb;
                rc = 0;
        }
[...]

Re: [PATCHv2] kvm-s390: fix potential array overrun in intercept handling

[Marcelo Tosatti]

On Thu, Jan 21, 2010 at 12:19:07PM +0100, Christian Borntraeger wrote:
> v2: apply Avis suggestions about ARRAY_SIZE.
> 
> kvm_handle_sie_intercept uses a jump table to get the intercept handler
> for a SIE intercept. Static code analysis revealed a potential problem:
> the intercept_funcs jump table was defined to contain (0x48 >> 2) entries,
> but we only checked for code > 0x48 which would cause an off-by-one
> array overflow if code == 0x48.
> 
> Use the compiler and ARRAY_SIZE to automatically set the limits.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Applied and queued for .33, CC: stable, thanks.

Re: [PATCHv2] kvm-s390: fix potential array overrun in intercept handling

[Alexander Graf]

On 21.01.2010, at 18:36, Marcelo Tosatti wrote:

> On Thu, Jan 21, 2010 at 12:19:07PM +0100, Christian Borntraeger wrote:
>> v2: apply Avis suggestions about ARRAY_SIZE.
>> 
>> kvm_handle_sie_intercept uses a jump table to get the intercept handler
>> for a SIE intercept. Static code analysis revealed a potential problem:
>> the intercept_funcs jump table was defined to contain (0x48 >> 2) entries,
>> but we only checked for code > 0x48 which would cause an off-by-one
>> array overflow if code == 0x48.
>> 
>> Use the compiler and ARRAY_SIZE to automatically set the limits.
>> 
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Applied and queued for .33, CC: stable, thanks.

Yes. Christian, please get this into 2.6.32-stable.

Alex