arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_regs'

PATCH In review Fengguang Wu 3 participants 4 messages Sep 11, 2012 – Sep 12, 2012

Subsystems (4)

x86 KVM Kernel janitors KVM x86

lore

Share & tools

lore

Fengguang Wu

author

2012-09-11 14:31 UTC #0 lore raw

PATCH 1/1 arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_regs'

Hi Avi,

In the kvm/next branch, sparse warns about

arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_regs' 9 <= 15

This is because the array definition is ctxt._regs[NR_VCPU_REGS] where NR_VCPU_REGS=9 for i386 and 17 for x86_64.

It could be fixed by changing the hard coded 16 to (NR_VCPU_REGS-1). And I wonder whether you actually want NR_VCPU_REGS here?

Thanks,
Fengguang
---

arch/x86/kvm/emulate.c +1 -1

@@ -228,7 +228,7 @@ static void writeback_registers(struct x
 {
 	unsigned reg;
 
-	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, 16)
+	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, NR_VCPU_REGS)
 		ctxt->ops->write_gpr(ctxt, reg, ctxt->_regs[reg]);
 }

Amos Kong

2012-09-12 05:58 UTC #1 lore raw

Re: [kvm:next 1/1] arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_r

Quoted 10 lines · Fengguang Wu

Hi Avi,
In the kvm/next branch, sparse warns about
arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_regs' 9 <= 15
This is because the array definition is ctxt._regs[NR_VCPU_REGS] where NR_VCPU_REGS=9 for i386 and 17 for x86_64.
It could be fixed by changing the hard coded 16 to (NR_VCPU_REGS-1).

Hi Fengguang,

You replaced 16 to NR_VCPU_REGS in your patch, not (NR_VCPU_REGS-1). I guess it's a mistake in your commitlog, right?

Quoted 18 lines

And I wonder whether you actually want NR_VCPU_REGS here?

Thanks,
Fengguang
---

arch/x86/kvm/emulate.c +1 -1

@@ -228,7 +228,7 @@ static void writeback_registers(struct x
  {
  	unsigned reg;
 
-	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, 16)
+	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, NR_VCPU_REGS)
 
 
  		ctxt->ops->write_gpr(ctxt, reg, ctxt->_regs[reg]);

Signature · 1 line

			Amos.

Fengguang Wu

author

2012-09-12 06:07 UTC #2 lore raw

Re: [kvm:next 1/1] arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_r

Quoted 10 lines · Amos Kong

Quoted 9 lines · Fengguang Wu
Hi Avi,
In the kvm/next branch, sparse warns about
arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_regs' 9 <= 15
This is because the array definition is ctxt._regs[NR_VCPU_REGS] where NR_VCPU_REGS=9 for i386 and 17 for x86_64.

Quoted 6 lines

Quoted 1 line
It could be fixed by changing the hard coded 16 to (NR_VCPU_REGS-1).
Hi Fengguang,
You replaced 16 to NR_VCPU_REGS in your patch, not (NR_VCPU_REGS-1). I guess it's a mistake in your commitlog, right?

16 = (NR_VCPU_REGS-1). So I mean, if replacing 16 with (NR_VCPU_REGS-1), there will be no behavior change for the x86_64 case. However I *suspect* the right value is (NR_VCPU_REGS), as I said in the below sentence.

Quoted 1 line

Quoted 1 line
And I wonder whether you actually want NR_VCPU_REGS here?

For your convenience, here is the relevant code for NR_VCPU_REGS:

        enum kvm_reg {
                VCPU_REGS_RAX = 0,
                VCPU_REGS_RCX = 1,
                VCPU_REGS_RDX = 2,
                VCPU_REGS_RBX = 3,
                VCPU_REGS_RSP = 4,
                VCPU_REGS_RBP = 5,
                VCPU_REGS_RSI = 6,
                VCPU_REGS_RDI = 7,
        #ifdef CONFIG_X86_64
                VCPU_REGS_R8 = 8,
                VCPU_REGS_R9 = 9,
                VCPU_REGS_R10 = 10,
                VCPU_REGS_R11 = 11,
                VCPU_REGS_R12 = 12,
                VCPU_REGS_R13 = 13,
                VCPU_REGS_R14 = 14,
                VCPU_REGS_R15 = 15,
        #endif 
                VCPU_REGS_RIP,
=>             NR_VCPU_REGS
        };

Thanks,
Fengguang

Quoted 17 lines

Quoted 13 lines

arch/x86/kvm/emulate.c +1 -1

@@ -228,7 +228,7 @@ static void writeback_registers(struct x
 {
 	unsigned reg;
 
-	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, 16)
+	for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, NR_VCPU_REGS)
 
 
 		ctxt->ops->write_gpr(ctxt, reg, ctxt->_regs[reg]);

}

Signature · 1 line

			Amos.

Avi Kivity

2012-09-12 07:37 UTC #3 lore raw

Re: [kvm:next 1/1] arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_r

Quoted 41 lines · Fengguang Wu

Quoted 10 lines · Amos Kong
Quoted 9 lines · Fengguang Wu
Hi Avi,
In the kvm/next branch, sparse warns about
arch/x86/kvm/emulate.c:232 writeback_registers() error: buffer overflow 'ctxt->_regs' 9 <= 15
This is because the array definition is ctxt._regs[NR_VCPU_REGS] where NR_VCPU_REGS=9 for i386 and 17 for x86_64.
Quoted 6 lines
Quoted 1 line
It could be fixed by changing the hard coded 16 to (NR_VCPU_REGS-1).
Hi Fengguang,
You replaced 16 to NR_VCPU_REGS in your patch, not (NR_VCPU_REGS-1). I guess it's a mistake in your commitlog, right?
16 = (NR_VCPU_REGS-1). So I mean, if replacing 16 with (NR_VCPU_REGS-1), there will be no behavior change for the x86_64 case. However I *suspect* the right value is (NR_VCPU_REGS), as I said in the below sentence.
Quoted 1 line
Quoted 1 line
And I wonder whether you actually want NR_VCPU_REGS here?
For your convenience, here is the relevant code for NR_VCPU_REGS:
        enum kvm_reg {
                VCPU_REGS_RAX = 0,
                VCPU_REGS_RCX = 1,
                VCPU_REGS_RDX = 2,
                VCPU_REGS_RBX = 3,
                VCPU_REGS_RSP = 4,
                VCPU_REGS_RBP = 5,
                VCPU_REGS_RSI = 6,
                VCPU_REGS_RDI = 7,
        #ifdef CONFIG_X86_64
                VCPU_REGS_R8 = 8,
                VCPU_REGS_R9 = 9,
                VCPU_REGS_R10 = 10,
                VCPU_REGS_R11 = 11,

Quoted 8 lines

                VCPU_REGS_R12 = 12,
                VCPU_REGS_R13 = 13,
                VCPU_REGS_R14 = 14,
                VCPU_REGS_R15 = 15,
        #endif 
                VCPU_REGS_RIP,
=>             NR_VCPU_REGS
        };

The right value is NR-1, since the loop excludes RIP.

Note the warning does not point to an actual error, since the high bits of regs_dirty will be clear on i386.

Signature · 1 line

error compiling committee.c: too many arguments to function