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: <58dbcb3c-5d5a-4f81-ac42-494b1fcaf932@t-8ch.de>
Date: Sat, 1 Feb 2025 11:41:58 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Willy Tarreau <w@....eu>
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 2025-02-01 11:34:38+0100, Willy Tarreau wrote:
> 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.

Point one is true.
Point two not so much. It is fine to have multiple directories open at
the same time. All state is kept inside the kernel.
Only the *current* return value is in the static buffer.
So multithreading wouldn't work, but nolibc doesn't support that
anyways.

> 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.

Ack.

> > 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.

Ack. Now that you suggest it, it seems obvious.

> > 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.

It comes from linux/limits.h. I don't think it can ever go away.

> > +	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.

If NAME_MAX changes it would be a userspace regression IMO.
I'll add a comment.


Thanks!
Thomas