linux-kernel - Re: [PATCH net-next v2 3/4] netconsole: Dynamic allocation of userdata buffer

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: <ucjifexudkswvaef5c25hbzszdnzsnx3drdaqkf7ytdpi6qzk6@pd46ih2slt3w>
Date: Fri, 14 Nov 2025 05:04:56 -0800
From: Breno Leitao <leitao@...ian.org>
To: Gustavo Luiz Duarte <gustavold@...il.com>
Cc: Andre Carvalho <asantostc@...il.com>, Simon Horman <horms@...nel.org>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/4] netconsole: Dynamic allocation of
 userdata buffer

On Thu, Nov 13, 2025 at 08:42:20AM -0800, Gustavo Luiz Duarte wrote:
> @@ -875,45 +875,61 @@ static ssize_t userdatum_value_show(struct config_item *item, char *buf)
>  	return sysfs_emit(buf, "%s\n", &(to_userdatum(item)->value[0]));
>  }
>  
> -static void update_userdata(struct netconsole_target *nt)
> +static int update_userdata(struct netconsole_target *nt)
>  {
> +	struct userdatum *udm_item;
> +	struct config_item *item;
>  	struct list_head *entry;
> -	int child_count = 0;
> +	char *old_buf = NULL;
> +	char *new_buf = NULL;
>  	unsigned long flags;
> +	int offset = 0;
> +	int len = 0;
>  
> -	spin_lock_irqsave(&target_list_lock, flags);
> -
> -	/* Clear the current string in case the last userdatum was deleted */
> -	nt->userdata_length = 0;
> -	nt->userdata[0] = 0;
> -
> +	/* Calculate buffer size */

Please create a function for this one.

>  	list_for_each(entry, &nt->userdata_group.cg_children) {
> -		struct userdatum *udm_item;
> -		struct config_item *item;
> -
> -		if (child_count >= MAX_USERDATA_ITEMS) {
> -			spin_unlock_irqrestore(&target_list_lock, flags);
> -			WARN_ON_ONCE(1);
> -			return;
> +		item = container_of(entry, struct config_item, ci_entry);
> +		udm_item = to_userdatum(item);
> +		/* Skip userdata with no value set */
> +		if (udm_item->value[0]) {
> +			len += snprintf(NULL, 0, " %s=%s\n", item->ci_name,
> +					udm_item->value);
>  		}
> -		child_count++;
> +	}
> +
> +	WARN_ON_ONCE(len > MAX_EXTRADATA_ENTRY_LEN * MAX_USERDATA_ITEMS);

If we trigger this WARN_ON_ONCE, please return, and do not proceed with
the buffer replacement.

> +
> +	/* Allocate new buffer */
> +	if (len) {
> +		new_buf = kmalloc(len + 1, GFP_KERNEL);
> +		if (!new_buf)
> +			return -ENOMEM;
> +	}
>  
> +	/* Write userdata to new buffer */
> +	list_for_each(entry, &nt->userdata_group.cg_children) {
>  		item = container_of(entry, struct config_item, ci_entry);
>  		udm_item = to_userdatum(item);
> -
>  		/* Skip userdata with no value set */
> -		if (strnlen(udm_item->value, MAX_EXTRADATA_VALUE_LEN) == 0)
> -			continue;
> -
> -		/* This doesn't overflow userdata since it will write
> -		 * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
> -		 * checked to not exceed MAX items with child_count above
> -		 */
> -		nt->userdata_length += scnprintf(&nt->userdata[nt->userdata_length],
> -						 MAX_EXTRADATA_ENTRY_LEN, " %s=%s\n",
> -						 item->ci_name, udm_item->value);
> +		if (udm_item->value[0]) {
> +			offset += scnprintf(&new_buf[offset], len + 1 - offset,
> +					    " %s=%s\n", item->ci_name,
> +					    udm_item->value);
> +		}
>  	}
> +
> +	WARN_ON_ONCE(offset != len);

if we hit the warning above, then offset < len, and we are wrapping some
item, right?

> +
> +	/* Switch to new buffer and free old buffer */
> +	spin_lock_irqsave(&target_list_lock, flags);
> +	old_buf = nt->userdata;
> +	nt->userdata = new_buf;
> +	nt->userdata_length = len;

This should be nt->userdata_length = offset, supposing the scnprintf got
trimmed, and the WARN_ON_ONCE above got triggered. Offset is the lenght
that was appened to new_buf.

>  	spin_unlock_irqrestore(&target_list_lock, flags);
> +
> +	kfree(old_buf);
> +
> +	return 0;
>  }

This seems all safe. update_userdata() is called with never called in
parallel, given it should be called with dynamic_netconsole_mutex, and
nt-> operations are protected by target_list_lock.

The only concern is nt->userdata_length = offset (instead of len).

>  
>  static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
> @@ -937,7 +953,9 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
>  
>  	ud = to_userdata(item->ci_parent);
>  	nt = userdata_to_target(ud);
> -	update_userdata(nt);
> +	ret = update_userdata(nt);
> +	if (ret < 0)
> +		goto out_unlock;
>  	ret = count;
>  out_unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
> @@ -1193,7 +1211,10 @@ static struct configfs_attribute *netconsole_target_attrs[] = {
>  
>  static void netconsole_target_release(struct config_item *item)
>  {
> -	kfree(to_target(item));
> +	struct netconsole_target *nt = to_target(item);

Thinking about this now, I suppose netconsole might be reading this in
parallel, and then we are freeing userdata mid-air.

Don't we need the target_list_lock in here ?

--
pw-bot: cr