sysctl.name false positive

[asiciliano]

Summary

https://bugs.freebsd.org/237778

The magical "sysctl.name" (0.1), gets the 'string name' from the 'int id' of a node, It returns 0 (false positive) and builds a fake name for a non-existent OID. The problem is reproducible by the attached file 'bad_name.c' PR237778 comment 1 (or bad_name.c).

We use 'sysctl.name' sysctl_sysctl_name() with '/sbin/sysctl -a' and '/sbin/sysctl -aN' (always positive case: existing node), I found it while I was writing a "sysctl library" then I should consider also the negative case (the others internal/magical OIDs sysctl.{oiddesc|oidfmt|oidlabel} return an error for a non-existent node).

Implementation note (https://gitlab.com/alfix/kernel-sysctlmibinfo/tree/master/bad_name_testing)

sysctl.{oiddesc|oidfmt|oidlabel} call sysctl_find_oid() to search the node and check the DORMANT and/or DYING flags, then my first patch 'patch_1' was just a call to sysctl_find_oid() and sysctl_search_oid() to build the path and the name. Unfortunately this elegant implementation is not efficient (2 visits of the tree, patch_1/time_*. txt).

The final solution is "merging" sysctl_find_oid() and sysctl_search_oid(): patch_2/kern_sysctl.c, test plan below.

Test Plan

Testing: https://gitlab.com/alfix/kernel-sysctlmibinfo/tree/master/bad_name_testing (you could install sysutils/nsysctl)

testing_patch.sh

#! /bin/sh

d=${1:-`pwd`};shift

~/kernel-sysctlmibinfo/bad_name_testing/bad_name > ${d}/bad_name.txt
~/nsysctl/nsysctl -aN > ${d}/nsysctl_aN.txt
~/nsysctl/nsysctl -aIN > ${d}/nsysctl_aIN.txt
sysctl -aN > ${d}/sysctl_aN.txt

counter=0
while [ $counter -lt 10 ]
do
	time -a -o ${d}/time_nsysctl_aN.txt ~/nsysctl/nsysctl -aN 
	time -a -o ${d}/time_nsysctl_aIN.txt ~/nsysctl/nsysctl -aIN 
	time -a -o ${d}/time_sysctl_aN.txt sysctl -aN
	counter=`expr $counter + 1`
done

% testing_patch ./old
% cat old/bad_name.txt 
id: [5]: 1.1.100.500.1000
error: 0, buflen: 25
error: 0, buflen: 25, name: kern.ostype.100.500.1000

false positive and fake name, apply the patch and compile the kernel

% testing_patch ./patch2
/% cat patch_2//bad_name.txt
id: [5]: 1.1.100.500.1000
error: -1, buflen: 0
error: -1, buflen: 0, name:

false positive fixed

% diff old/sysctl_aN.txt patch_2/sysctl_aN.txt
% diff old/nsysctl_aN.txt patch_2/nsysctl_aN.txt
% diff old/nsysctl_aIN.txt patch_2/nsysctl_aIN.txt

Positve case OK

% cat old/time_sysctl_aN.txt 
        0.08 real         0.01 user         0.04 sys
        0.07 real         0.00 user         0.04 sys
        0.06 real         0.01 user         0.03 sys
        0.07 real         0.00 user         0.05 sys
        0.06 real         0.00 user         0.03 sys
        0.06 real         0.01 user         0.02 sys
        0.05 real         0.00 user         0.04 sys
        0.08 real         0.01 user         0.04 sys
        0.04 real         0.00 user         0.04 sys
        0.06 real         0.00 user         0.03 sys
% cat patch_2/time_sysctl_aN.txt
        0.07 real         0.02 user         0.03 sys
        0.06 real         0.00 user         0.02 sys
        0.07 real         0.03 user         0.02 sys
        0.04 real         0.00 user         0.03 sys
        0.08 real         0.03 user         0.02 sys
        0.06 real         0.02 user         0.02 sys
        0.07 real         0.00 user         0.04 sys
        0.07 real         0.00 user         0.05 sys
        0.07 real         0.00 user         0.03 sys
        0.05 real         0.01 user         0.02 sys

Efficiency Testing OK

[cem]

So, I think this was an intentional feature and there are automatically generated numerical nodes which are not actually present in the tree structure.

I don't know if any of those are after non-CTLTYPE_NODE tree nodes, but you'd have to go looking to determine that.

I also don't know if there are other restrictions we can place on the namespace in order to ENOENT more appropriately. You'd have to take a survey of these magic numeric pseudo-nodes first. That's probably a good next step.

sys/kern/kern_sysctl.c

1025

Are we missing a vslock() here (not new in this change)? It seems like we really should't want to allow arbitrary userspace processes to take the sysctl tree global rmlock read-locked with a paged out output buffer + SYSCTL_OUT.

1050

DORMANT

[asiciliano]

In D20205#435565, @cem wrote:

So, I think this was an intentional feature and there are automatically generated numerical nodes which are not actually present in the tree structure.

Thank you for your comment,

Of course it is an intentional feature (char buf[10]; ...if (!lsp) { snprintf(buf,sizeof(buf),"%d",*name); ...}), anyway /sbin/sysctl does't use it: 'sysctl.name' receives only existent nodes (checked/found by 'sysctl.next'); obviously we want to print the name of real nodes.

I don't know if any of those are after non-CTLTYPE_NODE tree nodes, but you'd have to go looking to determine that.

It seems after the first fault or non-CTLTYPE_NODE, sysctl_sysctl_name() builds the name of the node dependig on the remandig input (int *arg1) no node is considered. Another false positive: some node has a number in the name, e.g. "hw.dri.X", then sysctl.name returns "true" end builds "hw.dri.X" for each X.

I also don't know if there are other restrictions we can place on the namespace in order to ENOENT more appropriately.

I' ll add new restrictions updating the diff. Guidelines: using sysctl_search_oid() to find the path of the node (then the name), comparing the output of 'sysctl.name' with 'sysctl.name2oid' and, possibly, adding the checks of 'sysctl.next' (e.g. SKIP).

You'd have to take a survey of these magic numeric pseudo-nodes first. That's probably a good next step.

My personal opinion:
sysctl_sysctl_name() was added by this commit 12623 23 years ago, it is substantially unchanged today, I think it preservers the intention of a SNMP-compatible interface (old Log Messages) then numeric psuedo-nodes (e.g., "kern.ostype.0" "kern.ostype.1.0") make sense, however sysctl-SNMP has not been implemented hackers@ ML; disclaimer :-) I am not a SNMP expert.

... Anyway, I have not a strong opinion about the output of 'sysctl.name', if you think the pseudo-nodes are a feature we could close this review.

sys/kern/kern_sysctl.c

1025

Nice comment (a lock is always an interesting topic), currently all the "undocumented interface" takes the sysctl tree lock, I would open a new review to discuss it properly and to make all the interface uniform.

1050

sysctl_search_oid() doesn't check if CTLTYPE_NODE is DORMANT, it seems a NODE couldn't be DORMANT sysctl_register_disabled_oid(), anyway I' ll add also a check for a DORMANT NODE to be sure.

[cem]

Hi Alfonso,

In D20205#436514, @alfix86_gmail.com wrote:

In D20205#435565, @cem wrote:

Of course it is an intentional feature (char buf[10]; ...if (!lsp) { snprintf(buf,sizeof(buf),"%d",*name); ...}), anyway /sbin/sysctl does't use it: 'sysctl.name' receives only existent nodes (checked/found by 'sysctl.next'); obviously we want to print the name of real nodes.

I am not sure it is totally unused. For example, it is invoked by libjail (jailparam_type(), jailparam_all()). security.jail.param has some oddball ABIs. E.g., param.ipv4 specifies an array value with ",a" in the format string. And param.allow.mount has a node with an empty name directly below the node.

Anyway, it is maybe difficult to automatically find all consumers, because the ABI is mostly invoked by raw number (0, 1) instead of a nicely defined name.

My personal opinion:
sysctl_sysctl_name() was added by this commit 12623 23 years ago, it is substantially unchanged today, I think it preservers the intention of a SNMP-compatible interface (old Log Messages) then numeric psuedo-nodes (e.g., "kern.ostype.0" "kern.ostype.1.0") make sense, however sysctl-SNMP has not been implemented hackers@ ML; disclaimer :-) I am not a SNMP expert.

Unfortunately the commit message doesn't really describe phk's aims. As Mark's email on the hacker's thread points out, one example pseudo-node is kern.proc.pid.<pid>. sysctl(8) doesn't know how to access it, but libutil, libkvm, and procstat do.

... Anyway, I have not a strong opinion about the output of 'sysctl.name', if you think the pseudo-nodes are a feature we could close this review.

ENOENT could probably be returned in more cases, but I'm not very confident what cases those are. I suspect changing this interface in this way doesn't provide a lot of benefit compared to the risk.

sys/kern/kern_sysctl.c

1025

I suspect my comment is misguided, and all of these SYSCTL_RLOCKs inside sysctl handler routines are redundant. All sysctls proceed through sysctl_root(), which RLOCKs the tree around the call to the handler anyway.

1050

Sorry, this remark was just intending to point out a mismatched assertion message — the check was for DORMANT but the print was for DYING, or vice versa, in the previous version of this patch. :-) Now they match.

[asiciliano]

This review is obsoleted by the <sysutils/sysctlinfo-kmod> port, so 'Abandon Revision'.