linux-kernel - Re: [RFC v2] kdbus: use LSM hooks to restrict ability to send file descriptors

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]

Message-ID: <CANq1E4SsWTCMYA58OofmNpdL-QvsTAZSLhi1C_08+BnXJHAsgQ@mail.gmail.com>
Date:	Mon, 21 Sep 2015 12:11:58 +0200
From:	David Herrmann <dh.herrmann@...il.com>
To:	Paul Osmialowski <p.osmialowsk@...sung.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Mack <daniel@...que.org>,
	David Herrmann <dh.herrmann@...glemail.com>,
	Djalal Harouni <tixxdz@...ndz.org>,
	Paul Moore <pmoore@...hat.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	LSM <linux-security-module@...r.kernel.org>,
	Lukasz Pawelczyk <l.pawelczyk@...sung.com>
Subject: Re: [RFC v2] kdbus: use LSM hooks to restrict ability to send file descriptors

Hi

On Mon, Sep 21, 2015 at 11:54 AM, Paul Osmialowski
<p.osmialowsk@...sung.com> wrote:
> The goal of this patch is to reproduce on kdbus the same behavior
> that is expressed by Unix Domain Sockets when it comes to restricting
> ability to pass opened file descriptors.
>
> Signed-off-by: Paul Osmialowski <p.osmialowsk@...sung.com>
> ---
>  ipc/kdbus/message.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
> index ae565cd..7f8aa35 100644
> --- a/ipc/kdbus/message.c
> +++ b/ipc/kdbus/message.c
> @@ -24,6 +24,7 @@
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/security.h>
>  #include <net/sock.h>
>
>  #include "bus.h"
> @@ -150,13 +151,19 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
>                 for (i = 0; i < gaps->n_fds; ++i) {
>                         int fd;
>
> -                       fd = get_unused_fd_flags(O_CLOEXEC);
> -                       if (fd < 0)
> -                               incomplete_fds = true;
> -
>                         WARN_ON(!gaps->fd_files[i]);
>
> -                       fds[n_fds++] = fd < 0 ? -1 : fd;
> +                       if (gaps->fd_files[i] &&

Please drop the WARN_ON above and this condition. It was fine before,
but here it just makes the code more complex, unnecessarily.

> +                                   security_file_receive(gaps->fd_files[i])) {
> +                               incomplete_fds = true;
> +                               fds[n_fds++] = -1;
> +                       } else {
> +                               fd = get_unused_fd_flags(O_CLOEXEC);
> +                               if (fd < 0)
> +                                       incomplete_fds = true;
> +
> +                               fds[n_fds++] = fd < 0 ? -1 : fd;
> +                       }
>                 }
>
>                 /*
> @@ -178,6 +185,16 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
>         for (i = 0; i < gaps->n_memfds; ++i) {
>                 int memfd;
>
> +               WARN_ON(!gaps->memfd_offsets[i]);
> +               WARN_ON(!gaps->memfd_files[i]);
> +
> +               if (gaps->memfd_files[i] &&

Same as above, just drop the WARN_ON and this condition.

> +                           security_file_receive(gaps->memfd_files[i])) {
> +                       incomplete_fds = true;
> +                       fds[n_fds++] = -1;
> +                       continue;
> +               }
> +

I don't see the point in protecting transmission of memfds. They're
treated as inline data. But I will not object it.

Patch looks good to me.

Thanks
David

>                 memfd = get_unused_fd_flags(O_CLOEXEC);
>                 if (memfd < 0) {
>                         incomplete_fds = true;
> @@ -193,10 +210,6 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
>                  * as usually there is no need to send more than one memfd per
>                  * message.
>                  */
> -
> -               WARN_ON(!gaps->memfd_offsets[i]);
> -               WARN_ON(!gaps->memfd_files[i]);
> -
>                 kvec.iov_base = &memfd;
>                 kvec.iov_len = sizeof(memfd);
>                 ret = kdbus_pool_slice_copy_kvec(slice, gaps->memfd_offsets[i],
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/