Bug 240573 - sysctl() does not return ENOMEM but silently truncate returned data
Summary: sysctl() does not return ENOMEM but silently truncate returned data
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords:
Depends on: 228432
Blocks:
  Show dependency treegraph
 
Reported: 2019-09-14 00:28 UTC by Ivan Rozhuk
Modified: 2022-01-24 14:23 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ivan Rozhuk 2019-09-14 00:28:30 UTC
int mib[4] = { CTL_KERN, KERN_PROC, KERN_PROC_FILEDESC, getpid() };

if (0 != sysctl(mib, 4, NULL, &buf_size, NULL, 0))
	return (errno);
buf = malloc(buf, buf_size);
newfd = open("/dev/null", O_RDONLY); /* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! */
if (0 != sysctl(mib, 4, buf, &buf_size, NULL, 0)) {
	if (ENOMEM != errno) {
		free(buf)
		return (errno);
	}
}

This code should fail, but it fill buf with struct kinfo_file, and there is no newfd.
No fail, no error code set, silent data truncation.


Probably sys/kern/kern_descrip.c: export_kinfo_to_sb()
...
		if (efbuf->remainder < kif->kf_structsize) {
			/* Terminate export. */
			efbuf->remainder = 0;
			return (0);
		}
...
should return here ENOMEM!?

I see hack to avoid missing fd for that in
lib/libutil/kinfo_getfile.c: kinfo_getfile():
...
len = len * 4 / 3;
...


Same for kern.ipc.posix_shm_list.

sys/kern/uipc_shm.c: sysctl_posix_shm_list()
			if (req->oldptr != NULL &&
			    kif.kf_structsize + curlen > req->oldlen)
				break;
error = ENOMEM; - before break missed.

hack:
usr.bin/posixshmcontrol/posixshmcontrol.c: list_shm()


sys/kern/kern_proc.c: kern_proc_vmmap_out()
...
		/* Halt filling and truncate rather than exceeding maxlen */
		if (maxlen != -1 && maxlen < kve->kve_structsize) {
			error = 0;
			vm_map_lock_read(map);
			break;
...
error = ENOMEM;?


And probably other places where exist buf size check and exit from loop before call sbuf_bcat().
Comment 1 Ivan Rozhuk 2022-01-16 21:57:31 UTC
At least kern.ipc.posix_shm_list and kern_proc_vmmap_out continue silently truncate data.
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2022-01-16 22:15:30 UTC
For kern_proc_vmmap_out(), we have:

		/* Halt filling and truncate rather than exceeding maxlen */
		if (maxlen != -1 && maxlen < kve->kve_structsize) {
			error = 0;
			vm_map_lock_read(map);
			break;
		} else if (maxlen != -1)
			maxlen -= kve->kve_structsize;

		if (sbuf_bcat(sb, kve, kve->kve_structsize) != 0)
			error = ENOMEM;

Note that when filling out info for the KERN_PROC_VMMAP sysctl, we have maxlen == -1.  So we just end up calling sbuf_bcat(), which will indeed return an error if the output buffer is too small.  I think there is no bug there.

I think you are right about sysctl_posix_shm_list(), we should return an error if req->oldptr is non-NULL and the buffer is truncated.  I believe it would be sufficient to just remove the referenced check, sbuf_bcat() will signal an error.
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2022-01-16 22:34:18 UTC
https://reviews.freebsd.org/D33912
Comment 4 Ivan Rozhuk 2022-01-17 01:07:25 UTC
(In reply to Mark Johnston from comment #2)

> For kern_proc_vmmap_out(), we have

I hope current/stable code handle it :)

PR was opened mostly because of KERN_PROC_FILEDESC, other things was found by quick search, I do not use/test it.
Probably there is more places/syscals that may silently truncate data.


> https://reviews.freebsd.org/D33912

Thanks!
Comment 5 commit-hook freebsd_committer freebsd_triage 2022-01-17 13:36:04 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=dc7526170d4cc62f35bafb25dd026399c612ed5b

commit dc7526170d4cc62f35bafb25dd026399c612ed5b
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2022-01-17 13:33:05 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-01-17 13:35:19 +0000

    posixshm: Report output buffer truncation from kern.ipc.posix_shm_list

    PR:             240573
    Reviewed by:    kib
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D33912

 sys/kern/uipc_shm.c | 3 ---
 1 file changed, 3 deletions(-)
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2022-01-17 13:51:39 UTC
(In reply to Ivan Rozhuk from comment #4)
I looked through the other KERN_PROC sysctls and did not find any occurrences of the problem.  I could have missed something of course.

kern_proc_filedesc_out() is a bit special.  It is used to provide info for an ELF coredump note as well as for KERN_PROC_FILEDESC.  During a coredump, the function is called twice: once to get the note size, and once to actually fill out the note.  NT_PROCSTAT_FILES notes contain path names.  So, if a referenced file is renamed between the two calls to kern_proc_filedesc_out(), we potentially have to truncate the new path to avoid corrupting the core dump.  The coredump code can reserve PATH_MAX bytes for each path to avoid this problem (this is enabled by the kern.coredump_pack_fileinfo sysctl), but that ends up bloating core dump sizes if a large number of files is open.

Most other sysctls don't have such a requirement, and so are less likely to have silent truncation bugs.
Comment 7 commit-hook freebsd_committer freebsd_triage 2022-01-24 14:21:41 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9795d85d2e73472e65d48b27a3c684f130722009

commit 9795d85d2e73472e65d48b27a3c684f130722009
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2022-01-17 13:33:05 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-01-24 14:16:37 +0000

    posixshm: Report output buffer truncation from kern.ipc.posix_shm_list

    PR:             240573
    Reviewed by:    kib
    Sponsored by:   The FreeBSD Foundation

    (cherry picked from commit dc7526170d4cc62f35bafb25dd026399c612ed5b)

 sys/kern/uipc_shm.c | 3 ---
 1 file changed, 3 deletions(-)
Comment 8 Mark Johnston freebsd_committer freebsd_triage 2022-01-24 14:23:32 UTC
I'm not aware of any other instances of sysctls truncating output buffers, please feel free to reopen if you find one.