netdev - Re: [PATCH net 1/2] net: bpfilter: restart bpfilter

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMArcTUp1=3880QXKSmUem=cwVT1Jc7MAxA88H4u2GHd0kX1-w@mail.gmail.com>
Date:   Sat, 15 Dec 2018 13:22:39 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Netdev <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Subject: Re: [PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

On Sat, 15 Dec 2018 at 09:58, David Miller <davem@...emloft.net> wrote:
>
> From: Taehee Yoo <ap420073@...il.com>
> Date: Wed, 12 Dec 2018 10:19:26 +0900
>
> > When bpfilter error occurred bpfilter_umh will be stopped via __stop_umh().
> > The bpfilter_umh() couldn't start again because there is no restart
> > routine.
> >
> > The section of bpfilter_umh_{start/end} is no longer .init.rodata
> > because these area should be reused in the restart routine. hence
> > the section name is changed to .bpfilter_umh.
> >
> > Test commands:
> >    $ iptables -vnL
> >    $ kill -9 <pid of bpfilter_umh>
> >    $ iptables -vnL
> >    [  480.045136] bpfilter: write fail -32
> >
> >    $ iptables -vnL
> >    iptables v1.8.1 (legacy): can't initialize iptables table `filter': No child processes
> >    Perhaps iptables or your kernel needs to be upgraded.
> >
> > Then, iptables command is always failed.
> >
> > Fixes: d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
> > Signed-off-by: Taehee Yoo <ap420073@...il.com>
>
> Thank you for this fix, but I am unsure if this is a complete solution.
>

Thank you for your review!

> First of all, you can only kill the bpfilter_umh as root right?
>

Yes, only root could kill bpfilter_umh process.

> It is a big problem to allow the userspace to kill the umh program
> because that will result in the pipes being leaked.  Normally the
> kernel takes down bpfilter_umh and calls fput() on the pipe file
> descriptors via shutdown_umh().
>
> In the kill -9 example above, that does not happen and that is why
> we get the fd leak.
>
> In this situation it will not reset info->pid to zero, and it also
> will leave bpfilter_process_socktop non-NULL.
>
> Therefore, it seems like the kernel has to perform these cleanup
> actions if the bpfilter_umh process dies (from kill -9, or just
> crashing).  And I think if you manage that properly it will fix this
> bug too.

If bpfilter_umh process is killed, shutdown_umh() is executed via __stop_umh().
because, __kernel_write() or kernel_read() will be failed in
__bpfilter_process_sockopt() if bpfilter_umh process had killed
or crashed. then, __bpfilter_process_sockopt() makes error message and
calls __stop_umh().
So the cleanup code is called when next iptables command is executed.

   $modprobe bpfilter
   $kill < pid of bpfilter_umh >
   $iptables -vnL   <-- stop_umh() is called and iptables command
fails at this point.
[  512.543626] bpfilter: write fail -32
   $iptables -vnL   <-- re-start routine will be called and iptables
command will success.

By any chance,  should the cleanup action immediately be called
when the bpfilter_umh process is killed?
If you think that I didn't understand correctly what you said,
please let me know.