lkml.org 
[lkml]   [2021]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 113/190] Revert "x86/hpet: Prevent potential NULL pointer dereference"
    On Fri, Apr 23, 2021 at 02:03:50AM -0700, Kees Cook wrote:
    > On Fri, Apr 23, 2021 at 01:33:07AM +0200, Thomas Gleixner wrote:
    > > On Wed, Apr 21 2021 at 12:49, Kees Cook wrote:
    > > > On Wed, Apr 21, 2021 at 02:59:48PM +0200, Greg Kroah-Hartman wrote:
    > > >> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
    > > >> index 08651a4e6aa0..0515a97bf6f5 100644
    > > >> --- a/arch/x86/kernel/hpet.c
    > > >> +++ b/arch/x86/kernel/hpet.c
    > > >> @@ -930,8 +930,6 @@ int __init hpet_enable(void)
    > > >> return 0;
    > > >>
    > > >> hpet_set_mapping();
    > > >> - if (!hpet_virt_address)
    > > >> - return 0;
    > > >>
    > > >> /* Validate that the config register is working */
    > > >> if (!hpet_cfg_working())
    > > >
    > > > FWIW, this patch looks harmless. It is checking for a failure in
    > > > hpet_set_mapping(), and avoids the following code from performing
    > > > 0-offset reads. hpet_set_mapping() is likely to never fail in real-world
    > > > situations. *shrug*
    > >
    > > 'likely never to fail' is clearly a receipe for disaster and you should
    > > know that.
    >
    > Of course -- I prefer to keep the sanity check. It just wasn't as good
    > as it could have been: it's not clear just by looking at the patch how
    > hpet_virt_address and hpet_set_mapping() are related.
    >
    > >
    > > > I think it would make more sense for the check to live in
    > > > hpet_cfg_working(), though.
    > >
    > > No. That does not make any sense at all.
    > >
    > > The proper change would have been to make hpet_set_mapping() return
    > > an error/success code and act on that.
    > >
    > > But that does _NOT_ make the patch invalid.
    > >
    > > I'm pretty sure that I looked at it and thought about the proper
    > > solution (see above) and then shrugged it off because of overload...
    >
    > Right, no, I was saying the original patch should stay. It shouldn't be
    > reverted.
    >
    > Greg, please drop this patch from the revert list.

    Now dropped, thanks for the review.

    greg k-h

    \
     
     \ /
      Last update: 2021-04-26 18:56    [W:4.052 / U:0.208 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site