linux-kernel - Re: [PATCH v2] audit: Send netlink ACK before setting connection in auditd

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: <102df7b7-a07a-475a-adb6-ae60453feb63@gmail.com>
Date:   Wed, 18 Oct 2023 15:11:28 +0300
From:   Rinat Gadelshin <rgadelsh@...il.com>
To:     Chris Riches <chris.riches@...anix.com>, audit@...r.kernel.org,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...hat.com>
Cc:     jonathan.davies@...anix.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] audit: Send netlink ACK before setting connection in
 auditd_set

Hello there!

On 18.10.2023 12:23, Chris Riches wrote:
> When auditd_set sets the auditd_conn pointer, audit messages can
> immediately be put on the socket by other kernel threads. If the backlog
> is large or the rate is high, this can immediately fill the socket
> buffer. If the audit daemon requested an ACK for this operation, a full
> socket buffer causes the ACK to get dropped, also setting ENOBUFS on the
> socket.
>
> To avoid this race and ensure ACKs get through, fast-track the ACK in
> this specific case to ensure it is sent before auditd_conn is set.
>
> Signed-off-by: Chris Riches <chris.riches@...anix.com>
>
> ---
>
>> I'm happier with the bool over the integer type for the @ack variable.
>> I'd suggest updating the patch and posting it again so we can review
>> it in full, but we weren't very far off last time so I think it should
>> be close, if not acceptable on the next revision.
> Here's the latest iteration of the suggested patch. I've done it via git
> send-email so it should apply cleanly.
>
>
>
>   kernel/audit.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 9bc0b0301198..20c6981490ad 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -488,15 +488,19 @@ static void auditd_conn_free(struct rcu_head *rcu)
>    * @pid: auditd PID
>    * @portid: auditd netlink portid
>    * @net: auditd network namespace pointer
> + * @skb: the netlink command from the audit daemon
> + * @ack: netlink ack flag, cleared if ack'd here
>    *
>    * Description:
>    * This function will obtain and drop network namespace references as
>    * necessary.  Returns zero on success, negative values on failure.
>    */
> -static int auditd_set(struct pid *pid, u32 portid, struct net *net)
> +static int auditd_set(struct pid *pid, u32 portid, struct net *net,
> +                      struct sk_buff *skb, bool *ack)
>   {
>   	unsigned long flags;
>   	struct auditd_connection *ac_old, *ac_new;
> +	struct nlmsghdr *nlh;
>   
>   	if (!pid || !net)
>   		return -EINVAL;
> @@ -508,6 +512,13 @@ static int auditd_set(struct pid *pid, u32 portid, struct net *net)
>   	ac_new->portid = portid;
>   	ac_new->net = get_net(net);
>   
> +	/* send the ack now to avoid a race with the queue backlog */
> +	if (*ack) {
> +		nlh = nlmsg_hdr(skb);
> +		netlink_ack(skb, nlh, 0, NULL);
> +		*ack = false;
> +	}
> +
>   	spin_lock_irqsave(&auditd_conn_lock, flags);
>   	ac_old = rcu_dereference_protected(auditd_conn,
>   					   lockdep_is_held(&auditd_conn_lock));
> @@ -1201,7 +1212,8 @@ static int audit_replace(struct pid *pid)
>   	return auditd_send_unicast_skb(skb);
>   }
>   
> -static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> +static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
> +			     bool *ack)
>   {
>   	u32			seq;
>   	void			*data;
> @@ -1294,7 +1306,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>   				/* register a new auditd connection */
>   				err = auditd_set(req_pid,
>   						 NETLINK_CB(skb).portid,
> -						 sock_net(NETLINK_CB(skb).sk));
> +						 sock_net(NETLINK_CB(skb).sk),
> +						 skb, ack);
>   				if (audit_enabled != AUDIT_OFF)
>   					audit_log_config_change("audit_pid",
>   								new_pid,
> @@ -1539,9 +1552,10 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>    * Parse the provided skb and deal with any messages that may be present,
>    * malformed skbs are discarded.
>    */
> -static void audit_receive(struct sk_buff  *skb)
> +static void audit_receive(struct sk_buff *skb)
>   {
>   	struct nlmsghdr *nlh;
> +	bool ack;
Maybe we should initialize 'ack' as 'true' here?
>   	/*
>   	 * len MUST be signed for nlmsg_next to be able to dec it below 0
>   	 * if the nlmsg_len was not aligned
> @@ -1554,9 +1568,12 @@ static void audit_receive(struct sk_buff  *skb)
>   
>   	audit_ctl_lock();
>   	while (nlmsg_ok(nlh, len)) {
> -		err = audit_receive_msg(skb, nlh);
> -		/* if err or if this message says it wants a response */
> -		if (err || (nlh->nlmsg_flags & NLM_F_ACK))
> +		ack = nlh->nlmsg_flags & NLM_F_ACK;
> +		err = audit_receive_msg(skb, nlh, &ack);
> +
> +		/* send an ack if the user asked for one and audit_receive_msg
> +		 * didn't already do it, or if there was an error. */
> +		if (ack || err)
>   			netlink_ack(skb, nlh, err, NULL);
>   
>   		nlh = nlmsg_next(nlh, &len);


Best regards
Rinat