Messages in this thread | | | From | Andy Lutomirski <> | Date | Mon, 11 Jan 2016 18:55:08 -0800 | Subject | Re: [GIT PULL v4.5] Fix INT1 recursion with unregistered breakpoints |
| |
On Mon, Jan 11, 2016 at 6:50 PM, Jeff Merkey <linux.mdb@gmail.com> wrote: > On 1/11/16, Andy Lutomirski <luto@amacapital.net> wrote: >> On Mon, Jan 11, 2016 at 6:26 PM, Jeff Merkey <linux.mdb@gmail.com> wrote: >>> On 1/11/16, Andy Lutomirski <luto@amacapital.net> wrote: >>>> On Mon, Jan 11, 2016 at 6:07 PM, Jeff Merkey <linux.mdb@gmail.com> >>>> wrote: >>>>> On 1/11/16, Andy Lutomirski <luto@amacapital.net> wrote: >>>>>> On Mon, Jan 11, 2016 at 5:30 PM, Jeff Merkey <linux.mdb@gmail.com> >>>>>> wrote: >>>>>>> On 1/11/16, Andy Lutomirski <luto@amacapital.net> wrote: >>>>>>>> On Mon, Jan 11, 2016 at 4:44 PM, Jeff Merkey <linux.mdb@gmail.com> >>>>>>>> wrote: >>>>>>>>> Hi Thomas, >>>>>>>>> >>>>>>>>> I agree with #2, we should clear the breakpoint. As for #1, if >>>>>>>>> there's an execute breakpoint it MUST be cleared or it will just >>>>>>>>> fire >>>>>>>>> off again when it sees the iretd from the int1 exception handler. >>>>>>>>> I >>>>>>>>> do use the breakpoint API Thomas, this showed up while debugging >>>>>>>>> and >>>>>>>>> testing the API with "lazy debug register switching". >>>>>>>>> >>>>>>>>> So do you want me to expand the patch and clear the breakpoint? >>>>>>>>> Just >>>>>>>>> give the word and I'll get busy and GIT -R- DONE. >>>>>>>> >>>>>>>> It seems to me that you're papering over some issue instead of >>>>>>>> fixing >>>>>>>> the root cause. If you're using the API, then either you're doing >>>>>>>> it >>>>>>>> wrong or the API is broken. Can you figure out which and fix it? >>>>>>>> >>>>>>>> --Andy >>>>>>>> >>>>>>> >>>>>>> Andy, >>>>>>> >>>>>>> Linux should not crash because someone triggered a breakpoint or one >>>>>>> got triggered due to a program leaving some bits lying in a read only >>>>>>> register (DR6) which for some strange reason someone in the linux >>>>>>> world decided could be used as local storage and to pass arguments >>>>>>> between subsystems - a register intel designed to be read from for >>>>>>> status. I did not design what's in that API, I have to live with >>>>>>> it. >>>>>> >>>>>> The API appears to work, though. Are you *sure* you're using it >>>>>> correctly? Are you telling the code in kernel/hw_breakpoint.c about >>>>>> your breakpoint? >>>>>> >>>>>>> So all I am asking is that we fix this issue. It does not matter >>>>>>> to my debugger is this is fixed or not in Linux, since I carry the >>>>>>> fix >>>>>>> in my patch, but it does matter to the overall robustness of Linux. >>>>>> >>>>>> Robust against what, exactly? What's the bug? >>>>>> >>>>>> I will grant that the comments about lazy dr7 switching are >>>>>> mystifying, and cleaning them up might be nice. But there's no >>>>>> adequate explanation of what the failure mode is, how to trigger it, >>>>>> or why your patch is a reasonable fix. As it stands, you're >>>>>> duplicating code. >>>>>> >>>>>> --Andy >>>>> >>>>> Andy, >>>>> >>>>> Couple of things: >>>>> >>>>> Would you like a copy of the test harness that creates this bug to >>>>> test for yourself? I previously posted it on the list. If you don't >>>>> have it, I'll provide it. >>>> >>>> If you can send a short, buildable thing that triggers it, I'll read it. >>>> >>>>> >>>>> Since the dr6 bits get shifted around, it doesn't matter if the >>>>> breakpoint was registered or not in the API because the broken handler >>>>> will call NULL bp structures and crash whether its registered or not. >>>>> >>>> >>>> And what exactly does this have to do with anything? Your patch is >>>> all about spurious breakpoints triggered by dr7 and should have >>>> nothing much to do with the value in dr6. Unless dr6 is missing a bit >>>> due to some issue, but you never suggested any problem like that. >>>> >>> >>> It's about setting the resume flag when an execute breakpoint occurs, no >>> matter >>> what caused the breakpoint. If is not set, the system will hang with >>> that processor >>> hung on the same execution address. You cannot have an int1 exception >>> path >>> that does not set the resume flag which is the case here -- there >>> should be no path >>> where this flag does not get set on an execute breakpoint. >> >> There are many, many ways that one can corrupt kernel state to break >> things. You could screw up IST state basically anywhere and crash. >> You could screw up GSBASE. You could poke bad values into pt_regs in >> a fast syscall and hit the infamous SYSRET failure. You can write a >> buggy .fault handler that returns success and doesn't actually do >> anything. And yes, you can set a bit in dr7 without telling the >> hw_breakpoint code about it and thus infinite loop. >> >> Meanwhile, you keep claiming that kernel has a bug and that the bug >> can't be triggered without out-of-tree code. In my book, that's not a >> bug. >> > > The handler that fails to set the resume flag is in tree code.
It's an unreachable code path.
> >> If you want to submit a nice clean patch to hw_breakpoint_handler to >> change the behavior on an unmatched breakpoint, then submit such a >> patch and justify why (a) the new behavior is better and (b) why it >> doesn't break any actual in-tree code. >> > > At last, a compromise -- accepted. In the meantime, put this patch in > to get rid of the crash. I'll code up another series and you can help me by > reviewing it and keeping me on my toes. >
No, because it still doesn't fix a bug *and* it's not a cleanup.
| |