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>] [day] [month] [year] [list]
Message-ID: <464FFAC2-3CB2-485D-B609-6C1722760DCE@oracle.com>
Date:   Fri, 16 Jun 2023 17:54:54 +0000
From:   Anjali Kulkarni <anjali.k.kulkarni@...cle.com>
To:     Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "david@...es.net" <david@...es.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "zbr@...emap.net" <zbr@...emap.net>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "johannes@...solutions.net" <johannes@...solutions.net>,
        "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>,
        "leon@...nel.org" <leon@...nel.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "socketcan@...tkopp.net" <socketcan@...tkopp.net>,
        "petrm@...dia.com" <petrm@...dia.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Liam Howlett <liam.howlett@...cle.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH v6 3/6] connector/cn_proc: Add filtering to fix some bugs



> On Jun 14, 2023, at 8:52 PM, Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com> wrote:
> 
> 
> 
> On Thu, Jun 15, 2023 at 5:12 AM Anjali Kulkarni <anjali.k.kulkarni@...cle.com> wrote:
> The current proc connector code has the foll. bugs - if there are more
> than one listeners for the proc connector messages, and one of them
> deregisters for listening using PROC_CN_MCAST_IGNORE, they will still get
> all proc connector messages, as long as there is another listener.
> 
> Another issue is if one client calls PROC_CN_MCAST_LISTEN, and another one
> calls PROC_CN_MCAST_IGNORE, then both will end up not getting any messages.
> 
> This patch adds filtering and drops packet if client has sent
> PROC_CN_MCAST_IGNORE. This data is stored in the client socket's
> sk_user_data. In addition, we only increment or decrement
> proc_event_num_listeners once per client. This fixes the above issues.
> 
> cn_release is the release function added for NETLINK_CONNECTOR. It uses
> the newly added netlink_release function added to netlink_sock. It will
> free sk_user_data.
> 
> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@...cle.com>
> ---
>  drivers/connector/cn_proc.c   | 53 ++++++++++++++++++++++++++++-------
>  drivers/connector/connector.c | 21 +++++++++++---
>  drivers/w1/w1_netlink.c       |  6 ++--
>  include/linux/connector.h     |  8 +++++-
>  include/uapi/linux/cn_proc.h  | 43 ++++++++++++++++------------
>  5 files changed, 96 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index ccac1c453080..84f38d2bd4b9 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -48,6 +48,21 @@ static DEFINE_PER_CPU(struct local_event, local_event) = {
>         .lock = INIT_LOCAL_LOCK(lock),
>  };
> 
> +static int cn_filter(struct sock *dsk, struct sk_buff *skb, void *data)
> +{
> +       enum proc_cn_mcast_op mc_op;
> +
> +       if (!dsk)
> +               return 0;
> +
> +       mc_op = ((struct proc_input *)(dsk->sk_user_data))->mcast_op;
> +
> +       if (mc_op == PROC_CN_MCAST_IGNORE)
> +               return 1;
> +
> +       return 0;
> +}
> +
>  static inline void send_msg(struct cn_msg *msg)
>  {
>         local_lock(&local_event.lock);
> @@ -61,7 +76,8 @@ static inline void send_msg(struct cn_msg *msg)
>          *
>          * If cn_netlink_send() fails, the data is not sent.
>          */
> -       cn_netlink_send(msg, 0, CN_IDX_PROC, GFP_NOWAIT);
> +       cn_netlink_send_mult(msg, msg->len, 0, CN_IDX_PROC, GFP_NOWAIT,
> +                            cn_filter, NULL);
> 
>         local_unlock(&local_event.lock);
>  }
> @@ -346,11 +362,9 @@ static void cn_proc_ack(int err, int rcvd_seq, int rcvd_ack)
>  static void cn_proc_mcast_ctl(struct cn_msg *msg,
>                               struct netlink_skb_parms *nsp)
>  {
> -       enum proc_cn_mcast_op *mc_op = NULL;
> -       int err = 0;
> -
> -       if (msg->len != sizeof(*mc_op))
> -               return;
> +       enum proc_cn_mcast_op mc_op = 0, prev_mc_op = 0;
> +       int err = 0, initial = 0;
> +       struct sock *sk = NULL;
> [Kalesh] Maintain reverse Xmas tree order.

Thanks for the review, will do.

> 
>         /* 
>          * Events are reported with respect to the initial pid
> @@ -367,13 +381,32 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
>                 goto out;
>         }
> 
> -       mc_op = (enum proc_cn_mcast_op *)msg->data;
> -       switch (*mc_op) {
> +       if (msg->len == sizeof(mc_op))
> +               mc_op = *((enum proc_cn_mcast_op *)msg->data);
> +       else
> +               return;
> +
> +       if (nsp->sk) {
> +               sk = nsp->sk;
> +               if (sk->sk_user_data == NULL) {
> +                       sk->sk_user_data = kzalloc(sizeof(struct proc_input),
> +                                                  GFP_KERNEL);
> [Kalesh] you need a check for memory allocation failure here.

Will add.

> +                       initial = 1;
> +               } else {
> +                       prev_mc_op =
> +                       ((struct proc_input *)(sk->sk_user_data))->mcast_op;
> +               }
> +               ((struct proc_input *)(sk->sk_user_data))->mcast_op = mc_op;
> +       }
> +
> +       switch (mc_op) {
>         case PROC_CN_MCAST_LISTEN:
> -               atomic_inc(&proc_event_num_listeners);
> +               if (initial || (prev_mc_op != PROC_CN_MCAST_LISTEN))
> +                       atomic_inc(&proc_event_num_listeners);
>                 break;
>         case PROC_CN_MCAST_IGNORE:
> -               atomic_dec(&proc_event_num_listeners);
> +               if (!initial && (prev_mc_op != PROC_CN_MCAST_IGNORE))
> +                       atomic_dec(&proc_event_num_listeners);
>                 break;
>         default:
>                 err = EINVAL;
> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
> index 48ec7ce6ecac..d1179df2b0ba 100644
> --- a/drivers/connector/connector.c
> +++ b/drivers/connector/connector.c
> @@ -59,7 +59,9 @@ static int cn_already_initialized;
>   * both, or if both are zero then the group is looked up and sent there.
>   */
>  int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid, u32 __group,
> -       gfp_t gfp_mask)
> +       gfp_t gfp_mask,
> +       int (*filter)(struct sock *dsk, struct sk_buff *skb, void *data),
> +       void *filter_data)
>  {
>         struct cn_callback_entry *__cbq;
>         unsigned int size;
> @@ -110,8 +112,9 @@ int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid, u32 __group,
>         NETLINK_CB(skb).dst_group = group;
> 
>         if (group)
> -               return netlink_broadcast(dev->nls, skb, portid, group,
> -                                        gfp_mask);
> +               return netlink_broadcast_filtered(dev->nls, skb, portid, group,
> +                                                 gfp_mask, filter,
> +                                                 (void *)filter_data);
>         return netlink_unicast(dev->nls, skb, portid,
>                         !gfpflags_allow_blocking(gfp_mask));
>  }
> @@ -121,7 +124,8 @@ EXPORT_SYMBOL_GPL(cn_netlink_send_mult);
>  int cn_netlink_send(struct cn_msg *msg, u32 portid, u32 __group,
>         gfp_t gfp_mask)
>  {
> -       return cn_netlink_send_mult(msg, msg->len, portid, __group, gfp_mask);
> +       return cn_netlink_send_mult(msg, msg->len, portid, __group, gfp_mask,
> +                                   NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(cn_netlink_send);
> 
> @@ -162,6 +166,14 @@ static int cn_call_callback(struct sk_buff *skb)
>         return err;
>  }
> 
> +static void cn_release(struct sock *sk, unsigned long *groups)
> +{
> +       if (groups && test_bit(CN_IDX_PROC - 1, groups)) {
> +               kfree(sk->sk_user_data);
> +               sk->sk_user_data = NULL;
> +       }
> +}
> +
>  /*
>   * Main netlink receiving function.
>   *
> @@ -249,6 +261,7 @@ static int cn_init(void)
>         struct netlink_kernel_cfg cfg = {
>                 .groups = CN_NETLINK_USERS + 0xf,
>                 .input  = cn_rx_skb,
> +               .release = cn_release,
>         };
> 
>         dev->nls = netlink_kernel_create(&init_net, NETLINK_CONNECTOR, &cfg);
> diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
> index db110cc442b1..691978cddab7 100644
> --- a/drivers/w1/w1_netlink.c
> +++ b/drivers/w1/w1_netlink.c
> @@ -65,7 +65,8 @@ static void w1_unref_block(struct w1_cb_block *block)
>                 u16 len = w1_reply_len(block);
>                 if (len) {
>                         cn_netlink_send_mult(block->first_cn, len,
> -                               block->portid, 0, GFP_KERNEL);
> +                                            block->portid, 0,
> +                                            GFP_KERNEL, NULL, NULL);
>                 }
>                 kfree(block);
>         }
> @@ -83,7 +84,8 @@ static void w1_reply_make_space(struct w1_cb_block *block, u16 space)
>  {
>         u16 len = w1_reply_len(block);
>         if (len + space >= block->maxlen) {
> -               cn_netlink_send_mult(block->first_cn, len, block->portid, 0, GFP_KERNEL);
> +               cn_netlink_send_mult(block->first_cn, len, block->portid,
> +                                    0, GFP_KERNEL, NULL, NULL);
>                 block->first_cn->len = 0;
>                 block->cn = NULL;
>                 block->msg = NULL;
> diff --git a/include/linux/connector.h b/include/linux/connector.h
> index 487350bb19c3..cec2d99ae902 100644
> --- a/include/linux/connector.h
> +++ b/include/linux/connector.h
> @@ -90,13 +90,19 @@ void cn_del_callback(const struct cb_id *id);
>   *             If @group is not zero, then message will be delivered
>   *             to the specified group.
>   * @gfp_mask:  GFP mask.
> + * @filter:     Filter function to be used at netlink layer.
> + * @filter_data:Filter data to be supplied to the filter function
>   *
>   * It can be safely called from softirq context, but may silently
>   * fail under strong memory pressure.
>   *
>   * If there are no listeners for given group %-ESRCH can be returned.
>   */
> -int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid, u32 group, gfp_t gfp_mask);
> +int cn_netlink_send_mult(struct cn_msg *msg, u16 len, u32 portid,
> +                        u32 group, gfp_t gfp_mask,
> +                        int (*filter)(struct sock *dsk, struct sk_buff *skb,
> +                                      void *data),
> +                        void *filter_data);
> 
>  /**
>   * cn_netlink_send - Sends message to the specified groups.
> diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
> index db210625cee8..6a06fb424313 100644
> --- a/include/uapi/linux/cn_proc.h
> +++ b/include/uapi/linux/cn_proc.h
> @@ -30,6 +30,30 @@ enum proc_cn_mcast_op {
>         PROC_CN_MCAST_IGNORE = 2
>  };
> 
> +enum proc_cn_event {
> +       /* Use successive bits so the enums can be used to record
> +        * sets of events as well
> +        */
> +       PROC_EVENT_NONE = 0x00000000,
> +       PROC_EVENT_FORK = 0x00000001,
> +       PROC_EVENT_EXEC = 0x00000002,
> +       PROC_EVENT_UID  = 0x00000004,
> +       PROC_EVENT_GID  = 0x00000040,
> +       PROC_EVENT_SID  = 0x00000080,
> +       PROC_EVENT_PTRACE = 0x00000100,
> +       PROC_EVENT_COMM = 0x00000200,
> +       /* "next" should be 0x00000400 */
> +       /* "last" is the last process event: exit,
> +        * while "next to last" is coredumping event
> +        */
> +       PROC_EVENT_COREDUMP = 0x40000000,
> +       PROC_EVENT_EXIT = 0x80000000
> +};
> +
> +struct proc_input {
> +       enum proc_cn_mcast_op mcast_op;
> +};
> +
>  /*
>   * From the user's point of view, the process
>   * ID is the thread group ID and thread ID is the internal
> @@ -44,24 +68,7 @@ enum proc_cn_mcast_op {
>   */
> 
>  struct proc_event {
> -       enum what {
> -               /* Use successive bits so the enums can be used to record
> -                * sets of events as well
> -                */
> -               PROC_EVENT_NONE = 0x00000000,
> -               PROC_EVENT_FORK = 0x00000001,
> -               PROC_EVENT_EXEC = 0x00000002,
> -               PROC_EVENT_UID  = 0x00000004,
> -               PROC_EVENT_GID  = 0x00000040,
> -               PROC_EVENT_SID  = 0x00000080,
> -               PROC_EVENT_PTRACE = 0x00000100,
> -               PROC_EVENT_COMM = 0x00000200,
> -               /* "next" should be 0x00000400 */
> -               /* "last" is the last process event: exit,
> -                * while "next to last" is coredumping event */
> -               PROC_EVENT_COREDUMP = 0x40000000,
> -               PROC_EVENT_EXIT = 0x80000000
> -       } what;
> +       enum proc_cn_event what;
>         __u32 cpu;
>         __u64 __attribute__((aligned(8))) timestamp_ns;
>                 /* Number of nano seconds since system boot */
> -- 
> 2.41.0
> 
> 
> 
> 
> -- 
> Regards,
> Kalesh A P

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ