Re: Potential bug in the dynamic linker?

On Fri, Nov 05, 2021 at 06:27:33PM +0100, obiwac wrote:
> Let me preface this by saying that I am in no way knowledgeable enough
> regarding the FreeBSD dynamic linker to know whether or not this is
> infact a bug or intended behaviour.
> 
> This program I'm working on, when compiled for FreeBSD, calls
> fdlopen(3) to load a dynamic library from memory. This is how I'm
> doing that more specifically:
> 
> // void* lib_bin, size_t lib_len
> 
> int fd = shm_open(SHM_ANON, O_RDWR, 0);
> ftruncate(fd, lib_len);
> 
> void* lib_mem = mmap(NULL, lib_len, PROT_WRITE, MAP_SHARED, fd, 0);
> memcpy(lib_mem, lib_bin, lib_len);
> 
> munmap(lib_mem, lib_len);
> 
> void* lib = fdlopen(fd, RTLD_LAZY);
> close(fd);
> 
> Running this on FreeBSD 13 works fine, FreeBSD 14, however, spits out
> this error:
> 
> Cannot fstatfs "<unknown>"
> 
> Digging around, I find, in libexec/rtld-elf/rtld.c:
> 
> /*
>  * but first, make sure that environment variables haven't been
>  * used to circumvent the noexec flag on a filesystem.
>  */
> if (dangerous_ld_env) {
>     if (fstatfs(fd, &fs) != 0) {
>         _rtld_error("Cannot fstatfs \"%s\"", printable_path(path));
>         return NULL;
>     }
>     if (fs.f_flags & MNT_NOEXEC) {
>         _rtld_error("Cannot execute objects on %s", fs.f_mntonname);
>         return NULL;
>     }
> }
> 
> And this is the first thing that seems weird to me. Why is it calling
> fstatfs(3) before checking if the file descriptor doesn't necessarily
> refer to a file which resides on a physical filesystem? It doesn't say
> so on the manpage, but, again, digging around, that's what the error
> returned by fstatfs(3), EINVAL, supposedly means.
Exactly what is missing from the man page?
How do you propose to check for MNT_NOEXEC without querying it with fstatfs()?

What could be improved there, IMO, is that fstatfs(2) error is considered
fatal, while it is arguably a reason to not check for MNT_NOEXEC.  As you
pointed out, fd might be not a file.

I will commit this change shortly.

> 
> Secondly, why then is dangerous_ld_env even set in the first place?
> Well, as of this commit (https://reviews.freebsd.org/D26352):
> 
> ld_dynamic_weak = ld_get_env_var(LD_DYNAMIC_WEAK) == NULL;
> 
> ...
> 
> dangerous_ld_env = libmap_disable || libmap_override != NULL ||
>     ld_library_path != NULL || ld_preload != NULL ||
>     ld_elf_hints_path != NULL || ld_loadfltr || ld_dynamic_weak;
This should by !ld_dynamic_weak.  I will commit the fix shortly.

> 
> Should this not be
> 
> ld_dynamic_weak = ld_get_env_var(LD_DYNAMIC_WEAK) != NULL;
No, this is wrong.  You are flipping the meaning of the env variable.

> 
> instead? Or is this actually intended and am I just not understanding
> the point of this?