linux-kernel - Re: [PATCH 2/2] tools/nolibc: add support for directory access

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: <20250201103438.GH5849@1wt.eu>
Date: Sat, 1 Feb 2025 11:34:38 +0100
From: Willy Tarreau <w@....eu>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Subject: Re: [PATCH 2/2] tools/nolibc: add support for directory access

On Thu, Jan 30, 2025 at 08:54:03PM +0100, Thomas Weißschuh wrote:
> From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> 
> Add an allocation-free implementation of readdir() and related
> functions. The implementation is modelled after the one for FILE.

I think you'd need to mention/remind the two important points that
come out of that choice, one being that DIR is a fake pointer that
instead stores ~fd so that it can be turned back to a valid FD, and
that subsequent readdir() calls will only work from the same file
unit since it relies on a local static storage.

Better have this visible in the commit message so that in the event
someone faces a difficulty due to this, they can easily find that it's
an on-purpose design choice.

> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> 
> ---
> I'm not entirely sure where to put it. It doesn't really belong into
> stdio.h, but that's where the FILE stuff is.
> sys.h wants alphabetical ordering, but IMO these functions should stick
> together.

My man pages suggest that userland code will include <dirent.h>, thus
I think it could be the moment to create it with that new code.

> diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> index 3892034198dd566d21a5cc0a9f67cf097d428393..1f275a0a7b6b2c6f1c15405d027c282bb77aa618 100644
> --- a/tools/include/nolibc/stdio.h
> +++ b/tools/include/nolibc/stdio.h
(...)
> +static __attribute__((unused))
> +struct dirent *readdir(DIR *dirp)
> +{
> +	static struct dirent dirent;
> +
> +	char buf[sizeof(struct linux_dirent64) + NAME_MAX];

I'm uncertain where NAME_MAX is defined, I haven't found it in the
nolibc sources, just double-checking that it's not just in your build
environment by accident.

> +	struct linux_dirent64 *ldir = (void *)buf;
> +	intptr_t i = (intptr_t)dirp;
> +	int fd, ret;
> +
> +	if (i >= 0) {
> +		SET_ERRNO(EBADF);
> +		return NULL;
> +	}
> +
> +	fd = ~i;
> +
> +	ret = getdents64(fd, ldir, sizeof(buf));
> +	if (ret == -1 || ret == 0)
> +		return NULL;
> +
> +	/*
> +	 * getdents64() returns as many entries as fit the buffer.
> +	 * readdir() can only return one entry at a time.
> +	 * Make sure the non-returned ones are not skipped.
> +	 */
> +	ret = lseek(fd, ldir->d_off, SEEK_SET);
> +	if (ret == -1)
> +		return NULL;
> +
> +	dirent = (struct dirent) {
> +		.d_ino = ldir->d_ino,
> +	};
> +	strlcpy(dirent.d_name, ldir->d_name, sizeof(dirent.d_name));

Just out of curiosity, could this copy fail, and if so, should we handle
it (e.g. NAME_MAX != 255) ? My guess here is that if it could almost never
fail and checking it would needlessly complicate the function, let's just
handle it with a comment for now. And if it cannot at all, let's mention
why on top of it as well.

Thanks,
Willy