From: Oliver O'Halloran <oohall@gmail.com> Date: 2016-01-25 06:56:14
I think this bug can only be triggered if the instruction to
simulate is malformed. The switch in the else case only handles
the zero and one case, but it extracts bits 4:1 from the
instruction word so it may be other values. It's pretty minor, but
a bug is a bug.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/lib/sstep.c | 1 +
1 file changed, 1 insertion(+)
From: Andrew Donnellan <hidden> Date: 2016-01-27 00:53:04
On 25/01/16 17:55, Oliver O'Halloran wrote:
I think this bug can only be triggered if the instruction to
simulate is malformed. The switch in the else case only handles
the zero and one case, but it extracts bits 4:1 from the
instruction word so it may be other values. It's pretty minor, but
a bug is a bug.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
The patch summary should probably be something along the lines of
"powerpc/sstep: fix switch fall-through when analysing malformed rld*
instructions" or similar. The rest of the message should have the more
specific details of the bug you're fixing.
In general, we always mention the affected subsystems in the patch
summary line and write both the summary line and the message so that
other developers can get a quick understanding of what the patch does
without actually needing to read the code. Keep in mind that commit
messages will show up in the git logs of every kernel developer, not
just powerpc people.
I had a cursory glance at the code and it's not obvious to me that this
is the correct way to deal with an invalid instruction. What happens
when you break out of the switch? It looks like it just ends up
returning 0, the same as any other instruction that isn't executed
directly in the analyse_instr() stage.
Is there anywhere else in the sstep code that deals well with malformed
instructions?
--
Andrew Donnellan Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com Australia Development Lab, Canberra
+61 2 6201 8874 (work) IBM Australia Limited
From: oliver <oohall@gmail.com> Date: 2016-01-27 05:29:04
The patch summary should probably be something along the lines of
"powerpc/sstep: fix switch fall-through when analysing malformed rld*
instructions" or similar. The rest of the message should have the more
specific details of the bug you're fixing.
In general, we always mention the affected subsystems in the patch
summary line and write both the summary line and the message so that other
developers can get a quick understanding of what the patch does without
actually needing to read the code. Keep in mind that commit messages will
show up in the git logs of every kernel developer, not just powerpc people.
That's fair.
I had a cursory glance at the code and it's not obvious to me that this
is the correct way to deal with an invalid instruction. What happens when
you break out of the switch? It looks like it just ends up returning 0, the
same as any other instruction that isn't executed directly in the
analyse_instr() stage.
Is there anywhere else in the sstep code that deals well with malformed
instructions?
When you break out of the switch the opcode type is marked as unknown and
when further attempts to parse the instruction fail it returns zero to
indicate failure. Also, many of the instructions handled by the function
are only valid in 64bit mode. For 32bit processors these instructions would
be illegal and the code that handles them is #ifdef`ed out when compiling
for 32 bit platforms so simply breaking out of the switch and letting it
propagate should be the right move here.
Oliver
On Wed, Jan 27, 2016 at 11:52 AM, Andrew Donnellan <
andrew.donnellan@au1.ibm.com> wrote:
quoted hunk
On 25/01/16 17:55, Oliver O'Halloran wrote:
quoted
I think this bug can only be triggered if the instruction to
simulate is malformed. The switch in the else case only handles
the zero and one case, but it extracts bits 4:1 from the
instruction word so it may be other values. It's pretty minor, but
a bug is a bug.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
The patch summary should probably be something along the lines of
"powerpc/sstep: fix switch fall-through when analysing malformed rld*
instructions" or similar. The rest of the message should have the more
specific details of the bug you're fixing.
In general, we always mention the affected subsystems in the patch summary
line and write both the summary line and the message so that other
developers can get a quick understanding of what the patch does without
actually needing to read the code. Keep in mind that commit messages will
show up in the git logs of every kernel developer, not just powerpc people.
--- a/arch/powerpc/lib/sstep.c
quoted
+++ b/arch/powerpc/lib/sstep.c
@@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op
I had a cursory glance at the code and it's not obvious to me that this is
the correct way to deal with an invalid instruction. What happens when you
break out of the switch? It looks like it just ends up returning 0, the
same as any other instruction that isn't executed directly in the
analyse_instr() stage.
Is there anywhere else in the sstep code that deals well with malformed
instructions?
--
Andrew Donnellan Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com Australia Development Lab, Canberra
+61 2 6201 8874 (work) IBM Australia Limited
From: Andrew Donnellan <hidden> Date: 2016-01-27 07:01:38
On 27/01/16 16:29, oliver wrote:
quoted
Is there anywhere else in the sstep code that deals well with malformed instructions?
When you break out of the switch the opcode type is marked as unknown
and when further attempts to parse the instruction fail it returns zero
to indicate failure. Also, many of the instructions handled by the
function are only valid in 64bit mode. For 32bit processors these
instructions would be illegal and the code that handles them is
#ifdef`ed out when compiling for 32 bit platforms so simply breaking out
of the switch and letting it propagate should be the right move here.
analyse_instr() returns 0 whenever it analyses but does not execute an
instruction - it's not a failure as such.
In emulate_step(), if analyse_instr() returns 0 it will test for a bunch
of instruction classes which require memory operations, and classes
which can't be single-stepped. UNKNOWN isn't handled specifically, so
it'll skip all that and return 0 at the end (meaning the step was not
successfully emulated - as opposed to -1, which is used for instructions
that are not allowed to be stepped).
This in turn is handled differently depending on whether emulate_step()
is invoked in the kprobes, uprobes or hw_breakpoint code.
Rather than breaking out and relying on behaviour later in the code, I'd
suggest either:
- creating a goto label for bad instructions that clearly sets the
type to UNKNOWN and returns 0 (and maybe adding some handling for that
in emulate_step(), raise some kind of nice big warning at the very least)
- make analyse_instr() return -1 on invalid instructions, which
emulate_step() will immediately propagate, then make sure that whoever
calls emulate_step() handles that appropriately
Andrew
--
Andrew Donnellan Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com Australia Development Lab, Canberra
+61 2 6201 8874 (work) IBM Australia Limited
From: Oliver O'Halloran <oohall@gmail.com> Date: 2016-02-15 23:29:11
There is a switch fallthough in instr_analyze() which can cause
an invalid instruction to be emulated as a different, valid,
instruction. The rld* (opcode 30) case extracts a sub-opcode from
bits 3:1 of the instruction word. However, the only valid values
of this field a 001 and 000. These cases are correctly handled,
but the others are not which causes execution to fall through
into case 31.
Breaking out of the switch causes the instruction to be marked as
unknown and allows the caller to deal with the invalid instruction
in a manner consistent with other invalid instructions.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/lib/sstep.c | 1 +
1 file changed, 1 insertion(+)
From: Andrew Donnellan <hidden> Date: 2016-02-16 01:01:09
On 16/02/16 10:28, Oliver O'Halloran wrote:
There is a switch fallthough in instr_analyze() which can cause
an invalid instruction to be emulated as a different, valid,
instruction. The rld* (opcode 30) case extracts a sub-opcode from
bits 3:1 of the instruction word. However, the only valid values
of this field a 001 and 000. These cases are correctly handled,
but the others are not which causes execution to fall through
into case 31.
Breaking out of the switch causes the instruction to be marked as
unknown and allows the caller to deal with the invalid instruction
in a manner consistent with other invalid instructions.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
The title should probably be "powerpc/sstep: fix switch fallthrough in
instruction emulation" to be consistent with our usual patch titling
practice. Please respin.
Apart from that, I'm reasonably convinced this is an appropriate fix:
Reviewed-by: Andrew Donnellan <redacted>
--
Andrew Donnellan Software Engineer, OzLabs
andrew.donnellan@au1.ibm.com Australia Development Lab, Canberra
+61 2 6201 8874 (work) IBM Australia Limited
From: Oliver O'Halloran <oohall@gmail.com> Date: 2016-02-16 06:32:57
There is a switch fallthough in instr_analyze() which can cause
an invalid instruction to be emulated as a different, valid,
instruction. The rld* (opcode 30) case extracts a sub-opcode from
bits 3:1 of the instruction word. However, the only valid values
of this field a 001 and 000. These cases are correctly handled,
but the others are not which causes execution to fall through
into case 31.
Breaking out of the switch causes the instruction to be marked as
unknown and allows the caller to deal with the invalid instruction
in a manner consistent with other invalid instructions.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
arch/powerpc/lib/sstep.c | 1 +
1 file changed, 1 insertion(+)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-05-10 21:48:46
On Tue, 2016-16-02 at 06:31:53 UTC, Oliver O'Halloran wrote:
There is a switch fallthough in instr_analyze() which can cause
an invalid instruction to be emulated as a different, valid,
instruction. The rld* (opcode 30) case extracts a sub-opcode from
bits 3:1 of the instruction word. However, the only valid values
of this field a 001 and 000. These cases are correctly handled,
but the others are not which causes execution to fall through
into case 31.
Breaking out of the switch causes the instruction to be marked as
unknown and allows the caller to deal with the invalid instruction
in a manner consistent with other invalid instructions.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>