linux-kernel - Re: [PATCH] gpio: cdev: fix a crash on line-request release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 25 May 2023 10:47:18 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpio: cdev: fix a crash on line-request release

On Wed, May 24, 2023 at 09:42:13PM +0200, Bartosz Golaszewski wrote:
> On Wed, May 24, 2023 at 6:36 AM Kent Gibson <warthog618@...il.com> wrote:
> >
> Interestingly enough, I did test it just like you and the "fix" seemed
> to address the issue. Upon a further look at the code, it's of course
> clear that the patch is wrong.
> 
> I wanted to debug the code to see what's happening exactly but it
> turned out that enabling the generation of DWARF data hid the issue as
> well even without any fix. It means that it's some kind of a memory
> corruption rather than a regular NULL-pointer dereference.
> 
> I'm not yet sure where the crash happens exactly other that it's in
> the irq domain code.
> 
> Anyway, I'll be back at it tomorrow.
> 

I was also playing with a patch for gpiomon to have it add POLLERR to
its poll() to see if it would notice the chip removal and exit.
It didn't, it just stayed blocked, but it DID made the crash go away
when I killed it.  No idea why that would be.  So yeah, weird things.

This was the patch, btw:

diff --git a/tools/gpiomon.c b/tools/gpiomon.c
index c2684c2..f4251fc 100644
--- a/tools/gpiomon.c
+++ b/tools/gpiomon.c
@@ -431,7 +431,7 @@ int main(int argc, char **argv)
                                   resolver->chips[i].path);
 
                pollfds[i].fd = gpiod_line_request_get_fd(requests[i]);
-               pollfds[i].events = POLLIN;
+               pollfds[i].events = POLLIN | POLLERR;
                gpiod_chip_close(chip);
        }
 
@@ -452,6 +452,9 @@ int main(int argc, char **argv)
                        if (pollfds[i].revents == 0)
                                continue;
 
+                       if (pollfds[i].revents & POLLERR)
+                               die_perror("error polling for events");
+
                        ret = gpiod_line_request_read_edge_events(requests[i],
                                         event_buffer, EVENT_BUF_SIZE);
                        if (ret < 0)


I expect to add that, or something along those lines, if hot removal of
chips from the kernel ever works like it should.
In the meantime it is just another curiosity.

Cheers,
Kent.