* [PATCH] Fix fall-through from case 30 (rld*) to case 31 @ 2016-01-25 6:55 Oliver O'Halloran 2016-01-27 0:52 ` Andrew Donnellan 2016-02-15 23:28 ` [PATCH] powerpc/lib/sstep.c - Fix emulation fall-through Oliver O'Halloran 0 siblings, 2 replies; 8+ messages in thread From: Oliver O'Halloran @ 2016-01-25 6:55 UTC (permalink / raw) To: linuxppc-dev; +Cc: Oliver O'Halloran 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(+) diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index dc885b3..e25f73c 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs, } } #endif + break; /* illegal instruction */ case 31: switch ((instr >> 1) & 0x3ff) { -- 2.5.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix fall-through from case 30 (rld*) to case 31 2016-01-25 6:55 [PATCH] Fix fall-through from case 30 (rld*) to case 31 Oliver O'Halloran @ 2016-01-27 0:52 ` Andrew Donnellan 2016-01-27 5:29 ` oliver 2016-02-15 23:28 ` [PATCH] powerpc/lib/sstep.c - Fix emulation fall-through Oliver O'Halloran 1 sibling, 1 reply; 8+ messages in thread From: Andrew Donnellan @ 2016-01-27 0:52 UTC (permalink / raw) To: Oliver O'Halloran, linuxppc-dev 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. > --- a/arch/powerpc/lib/sstep.c > +++ b/arch/powerpc/lib/sstep.c > @@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs, > } > } > #endif > + break; /* illegal instruction */ 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix fall-through from case 30 (rld*) to case 31 2016-01-27 0:52 ` Andrew Donnellan @ 2016-01-27 5:29 ` oliver 2016-01-27 7:00 ` Andrew Donnellan 0 siblings, 1 reply; 8+ messages in thread From: oliver @ 2016-01-27 5:29 UTC (permalink / raw) To: Andrew Donnellan; +Cc: linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 3533 bytes --] > 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: > 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. > > --- a/arch/powerpc/lib/sstep.c >> +++ b/arch/powerpc/lib/sstep.c >> @@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op >> *op, struct pt_regs *regs, >> } >> } >> #endif >> + break; /* illegal instruction */ >> > > 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 > > [-- Attachment #2: Type: text/html, Size: 5111 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix fall-through from case 30 (rld*) to case 31 2016-01-27 5:29 ` oliver @ 2016-01-27 7:00 ` Andrew Donnellan 0 siblings, 0 replies; 8+ messages in thread From: Andrew Donnellan @ 2016-01-27 7:00 UTC (permalink / raw) To: oliver; +Cc: linuxppc-dev On 27/01/16 16:29, oliver wrote: >>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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] powerpc/lib/sstep.c - Fix emulation fall-through 2016-01-25 6:55 [PATCH] Fix fall-through from case 30 (rld*) to case 31 Oliver O'Halloran 2016-01-27 0:52 ` Andrew Donnellan @ 2016-02-15 23:28 ` Oliver O'Halloran 2016-02-16 0:59 ` Andrew Donnellan 1 sibling, 1 reply; 8+ messages in thread From: Oliver O'Halloran @ 2016-02-15 23:28 UTC (permalink / raw) To: linuxppc-dev; +Cc: Oliver O'Halloran 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(+) diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index dc885b3..e25f73c 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs, } } #endif + break; /* illegal instruction */ case 31: switch ((instr >> 1) & 0x3ff) { -- 2.5.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] powerpc/lib/sstep.c - Fix emulation fall-through 2016-02-15 23:28 ` [PATCH] powerpc/lib/sstep.c - Fix emulation fall-through Oliver O'Halloran @ 2016-02-16 0:59 ` Andrew Donnellan 2016-02-16 6:31 ` [PATCH v2] powerpc/sstep.c " Oliver O'Halloran 0 siblings, 1 reply; 8+ messages in thread From: Andrew Donnellan @ 2016-02-16 0:59 UTC (permalink / raw) To: Oliver O'Halloran, linuxppc-dev 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 <andrew.donnellan@au1.ibm.com> -- Andrew Donnellan Software Engineer, OzLabs andrew.donnellan@au1.ibm.com Australia Development Lab, Canberra +61 2 6201 8874 (work) IBM Australia Limited ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] powerpc/sstep.c - Fix emulation fall-through 2016-02-16 0:59 ` Andrew Donnellan @ 2016-02-16 6:31 ` Oliver O'Halloran 2016-05-10 21:48 ` [v2] " Michael Ellerman 0 siblings, 1 reply; 8+ messages in thread From: Oliver O'Halloran @ 2016-02-16 6:31 UTC (permalink / raw) To: linuxppc-dev; +Cc: andrew.donnellan, Oliver O'Halloran 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(+) diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index dc885b3..e25f73c 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -925,6 +925,7 @@ int __kprobes analyse_instr(struct instruction_op *op, struct pt_regs *regs, } } #endif + break; /* illegal instruction */ case 31: switch ((instr >> 1) & 0x3ff) { -- 2.5.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [v2] powerpc/sstep.c - Fix emulation fall-through 2016-02-16 6:31 ` [PATCH v2] powerpc/sstep.c " Oliver O'Halloran @ 2016-05-10 21:48 ` Michael Ellerman 0 siblings, 0 replies; 8+ messages in thread From: Michael Ellerman @ 2016-05-10 21:48 UTC (permalink / raw) To: Oliver O'Halloran, linuxppc-dev Cc: Oliver O'Halloran, andrew.donnellan 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> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/ab66c8ca52f790d816e421d3b1 cheers ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-05-10 21:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-25 6:55 [PATCH] Fix fall-through from case 30 (rld*) to case 31 Oliver O'Halloran 2016-01-27 0:52 ` Andrew Donnellan 2016-01-27 5:29 ` oliver 2016-01-27 7:00 ` Andrew Donnellan 2016-02-15 23:28 ` [PATCH] powerpc/lib/sstep.c - Fix emulation fall-through Oliver O'Halloran 2016-02-16 0:59 ` Andrew Donnellan 2016-02-16 6:31 ` [PATCH v2] powerpc/sstep.c " Oliver O'Halloran 2016-05-10 21:48 ` [v2] " Michael Ellerman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.