linux-kernel - Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE

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: <c77e83a0-7587-3e07-81cb-6b185d1ce6aa@linux.com>
Date:   Sat, 29 Aug 2020 09:59:08 +0300
From:   Denis Efremov <efremov@...ux.com>
To:     Joe Perches <joe@...ches.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Kees Cook <keescook@...omium.org>,
        "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Julia Lawall <julia.lawall@...ia.fr>,
        Alex Dewar <alex.dewar90@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE
 buffers.

Hi,

On 8/29/20 1:52 AM, Joe Perches wrote:
> sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> used for outputting sysfs content requests and it's possible to
> overrun the buffer length.
> 
> Add a generic sysfs_emit mechanism that knows that the size of the
> temporary buffer and ensures that no overrun is done.
> 
> Signed-off-by: Joe Perches <joe@...ches.com>
> ---


It could be a good idea to update the docs to, i.e.:
https://www.kernel.org/doc/html/latest/filesystems/sysfs.html


>  fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  8 ++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index eb6897ab78e7..06a13bbd7080 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(sysfs_change_owner);
> +
> +/**
> + *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
> + *	@buf:	start of PAGE_SIZE buffer.
> + *	@pos:	current position in buffer
> + *              (pos - buf) must always be < PAGE_SIZE
> + *	@fmt:	format
> + *	@...:	arguments to format
> + *
> + *
> + * Returns number of characters written at pos.
> + */
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> +	va_list args;
> +	bool bad_pos = pos < buf;
> +	bool bad_len = (pos - buf) >= PAGE_SIZE;
> +	int len;
> +
> +	if (WARN(bad_pos || bad_len, "(pos < buf):%d (pos >= PAGE_SIZE):%d\n",
> +		 bad_pos, bad_len))
> +		return 0;
> +
> +	va_start(args, fmt);
> +	len = vscnprintf(pos, PAGE_SIZE - (pos - buf), fmt, args);
> +	va_end(args);
> +
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(sysfs_emit);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 34e84122f635..5a21d3d30016 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -329,6 +329,8 @@ int sysfs_groups_change_owner(struct kobject *kobj,
>  int sysfs_group_change_owner(struct kobject *kobj,
>  			     const struct attribute_group *groups, kuid_t kuid,
>  			     kgid_t kgid);
> +__printf(3, 4)
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...);
>  
>  #else /* CONFIG_SYSFS */
>  
> @@ -576,6 +578,12 @@ static inline int sysfs_group_change_owner(struct kobject *kobj,
>  	return 0;
>  }
>  
> +__printf(3, 4)
> +static inline int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> +	return 0;
> +}
> +
>  #endif /* CONFIG_SYSFS */
>  
>  static inline int __must_check sysfs_create_file(struct kobject *kobj,
> 

Thanks,
Denis