Restore access to "user.*" variables in sysctlbyname()

[se]

Summary

Commit d05b53e0baee7 removed access to the "user" variable tree by sysctlbyname().

This is not obvious since most commands use sysctl() with a numeric OID array instead of sysctlbyname() to access user.* variables.

One command that *is* affected is "whereis", but since the result of sysctlbyname("user.cs_path") is normally overridden by the value of the PATH environment variable, this issue is not detected.

The sysctl command uses sysctl() instead of sysctlbyname() and is not affected by this problem.

This review restores the previous implementation of sysctlbyname() and invokes it for the user sub-tree.

Test Plan

Test the result of the following C function call without and again after applying the patch included in this review:

char b[1024];
size_t len = sizeof(b);
sysctlbyname("user.cs_path", b, &len, NULL, 0);

Alternatively print the contents of the variable "b" at line 279 of usr.bin/whereis.c (after the second sysctlbyname() call).

[kaktus]

lib/libc/gen/sysctlbyname.c

46

I believe this can be wrapped in __predict_false because this case is rather unlikely.

[asiciliano]

char b[1024];
sysctlbyname("user.cs_path", b, 1024, NULL, 0);

1024 should be a passed like a pointer, example "valuelen":

int main()
{
	int error;
	char value[5000];
	size_t valuelen;

	printf("  ## sysctlbyname ##\n");
	valuelen = 5000;
	if (sysctlbyname("user.cs_path", &value, &valuelen, NULL, 0) == 0)
		printf("user.cs_path: %s\n", value);
	else
		printf("user.cs_path.: error\n");


	printf("  ## sysctlbyname_improved ##\n");
	valuelen =5000;
	if (sysctlbyname_improved("user.cs_path", &value, &valuelen, NULL, 0) == 0)
		printf("user.cs_path: %s\n", value);
	else
		printf("user.cs_path: error\n");

	return 0;
}

However, it is strange but real, sysctlbyname() returns 0 but does not set the "value", while sysctlbyname-improved sets "value" correctly:

  ## sysctlbyname ##
user.cs_path: 
  ## sysctlbyname_improved ##
user.cs_path: /usr/bin:/bin:/usr/sbin:/sbin

I would avoid a hardcode of the "user" subtree, maybe it is better to find the problem in kern_sysctl.c.

[se]

Updated diff with inlined function to fetch the user sub-tree. The common case is predicated with __predict_true as suggested by kaktus.

I'm not sure about phk's copyright - the beerware license had been removed when the function had been re-implemented.
This patch brings similar (but not identical) code back into the function. (Adding phk as reviewer for that reason.)

[se]

lib/libc/gen/sysctlbyname.c

46

Implemented as __predict_true of the negated condition in the updated patch.

[kaktus]

In D34171#773087, @alfix86_gmail.com wrote:

However, it is strange but real, sysctlbyname() returns 0 but does not set the "value", while sysctlbyname-improved sets "value" correctly:

Because your implementation is very different from this one.

I would avoid a hardcode of the "user" subtree, maybe it is better to find the problem in kern_sysctl.c.

Unfortunately the user.* tree is a synthetic one and implemented in very different way than others - see kern/kern_mib.c.

[se]

In D34171#773087, @alfix86_gmail.com wrote:

char b[1024];
sysctlbyname("user.cs_path", b, 1024, NULL, 0);

1024 should be a passed like a pointer, example "valuelen":

Yes, sorry, I had typed in this command from memory - my actual test code was correct and the buffer length was passed via a pointer.

int main()
{
	int error;
	char value[5000];
	size_t valuelen;

	printf("  ## sysctlbyname ##\n");
	valuelen = 5000;
	if (sysctlbyname("user.cs_path", &value, &valuelen, NULL, 0) == 0)
		printf("user.cs_path: %s\n", value);
	else
		printf("user.cs_path.: error\n");

The current implementation of sysctlbyname() sets value to an empty string and returns a value of 0.

printf(" sysctlbyname_improved \n");
valuelen =5000;
if (sysctlbyname_improved("user.cs_path", &value, &valuelen, NULL, 0) == 0)

		printf("user.cs_path: %s\n", value);

else

		printf("user.cs_path: error\n");

return 0;
}

I have looked at sysctlbyname_improved and it seems to bring back the logic that has been replaced a few years ago insofar as it requires multiple system calls per value returned.

However, it is strange but real, sysctlbyname() returns 0 but does not set the "value", while sysctlbyname-improved sets "value" correctly:

Yes, sysctlbyname("user.cs_path") returns 0 since the kernel contains the matching element name with a constant empty string as the value.

The actual value is provided by the sysctl() function in the C library - but currently only for sysctl(), not sysctlbyname().

  ## sysctlbyname ##
user.cs_path: 
  ## sysctlbyname_improved ##
user.cs_path: /usr/bin:/bin:/usr/sbin:/sbin

I would avoid a hardcode of the "user" subtree, maybe it is better to find the problem in kern_sysctl.c.

There is no problem in kern_sysctl.c.

The user sub-tree is already hard-coded in the C library (in gen/sysctl.c) and the patch refers to that function for sysctlbyname("user.*").

[asiciliano]

@se Ok, (I have not a strong opinion about it, just some idea) I took a look at sysctl a few years ago, surely something has changed.

[mjg]

The user.* case should look identical to what it used to prior to d05b53e0baee7670da54424738bda9b11c0668e7

[se]

In D34171#773290, @mjg wrote:

The user.* case should look identical to what it used to prior to d05b53e0baee7670da54424738bda9b11c0668e7

The proposed code is functionally equivalent, but somewhat simplified and more symmetric.

The code for oid[0] == CTL_USER in in sysctl() enforces namelen == 2, therefore it is not necessary to provide a larger OID array.
If this assumption (namelen == 2 for CTL_USER) would be invalidated by changes to sysctl(), then passing a 2 element array would make sysctlnametomib() fail.
(I.e. there would not be any security issue, and the failure would be detected due to the error return.)

If there is strong consensus that a return to the exact previous form is beneficial, I'd of course accept that (with namelen set to 2), but IMHO this is not the case.

[kaktus]

I agree with @se on this. There was no direct user case before introducing sysctlbyname(2), it worked as an by-effect of implementation of how sysctl(3) handled that.
IMO the issue is in how the user.* is implemented and maybe that should be fixed in the long term, but as a band-aid that version LGTM.

[se]

In D34171#773942, @kaktus wrote:

I agree with @se on this. There was no direct user case before introducing sysctlbyname(2), it worked as an by-effect of implementation of how sysctl(3) handled that.
IMO the issue is in how the user.* is implemented and maybe that should be fixed in the long term, but as a band-aid that version LGTM.

Since the user.* name space returns data that applies to the user-land, not the kernel, it is not possible to have a kernel only implementation.
Instead of substituting the "real" result in the C library after checking access rights in the kernel, it is also possible to store this user-land values in the kernel at system start-up.

The C library based implementation has the advantage, that the constants are actual hard-coded constants and not just write-once kernel variables (for the most part at least).

If there is consensus that the user.* variables should be writable by root at system start-up, I can prepare a kernel patch and a rc.d/sysctl_user script to perform the initialization.
As a result, the code in sysctl() and sysctlbyname() could be simplified to just return the kernel variable.

[mjg]

My point was that the previous code was known to work with user.* and I don't see any reason to change that bit, but I'm not going to insist.

lib/libc/gen/sysctlbyname.c

51

this should be nitems(real_oid)

[se]

In D34171#774009, @mjg wrote:

My point was that the previous code was known to work with user.* and I don't see any reason to change that bit, but I'm not going to insist.

The previous code has been replaced by the one currently in -CURRENT to replace multiple system calls with a single (new) system call.
But this optimization lost access to the special cases in sysctl() in the C library, and I want to restore that code only for the case of "user.*", leaving the fast path intact for all other variables.

There was an intermediate version to allow for new user-land on old kernels that did not provide the added system call. I had not known about that version when I created the review, but the code I wrote is nearly identical to the letter to what used to be there (where applicable).

That intermediate version was:

int
sysctlbyname(const char *name, void *oldp, size_t *oldlenp,
    const void *newp, size_t newlen)
{
        int oid[CTL_MAXNAME];
        size_t len;

        if (__getosreldate() >= SYSCTLBYNAME_OSREL) {
                len = strlen(name);
                return (__sysctlbyname(name, len, oldp, oldlenp, newp,
                    newlen));
        }
        len = nitems(oid);
        if (sysctlnametomib(name, oid, &len) == -1)
                return (-1);
        return (sysctl(oid, len, oldp, oldlenp, newp, newlen));
}

If you compare that function with the proposed one you'll find that I'm in fact going back to that intermediate version, but with the test for "__getosreldate() >= SYSCTLBYNAME_OSREL" replaced by the test for a "user.*" variable being requested. (I'll rename "real_oid to oid to make the code even more similar.)

I have introduced an else block to make the control flow more obvious - the return in the first case implies skipping the rest of the function, but I prefer to make this explicit.
But this is a purely cosmetic change - if this was the last bit people were arguing about I'd paint the shed black (and accept the IMHO less clear code without the else clause).

lib/libc/gen/sysctlbyname.c

51

OK, I can change that - it will obviously return 2 due to the way the OID array is defined and the assumption that there are exactly 2 elements in the user sub-tree are hard-coded in a number of places. Since this value always has been and always will be "2" I think the numeric constant is appropriate, here, to explicitly show this fact.

[se]

Small non-functional changes based on comments received.