Details

Reviewers

oshogbo
emaste
cem
def

Group Reviewers

capsicum

Commits

rS342699: Capsicumize savecore(8).

Summary

This is relatively straightforward. In main(), savecore builds up a
list of device files to search for kernel dump headers. We use
cap_fileargs to open each device in turn. savecore saves a number of
files in a directory (cwd by default) if a kernel dump is found; we
use a directory fd, savedirfd, to reference that directory.

I considered making savedirfd a global var rather than plumbing it
through everything. I have a feeling it'll someday be useful to parameterize
this code by savedir, though, so I didn't do that. An alternative is to define a
struct savecorectx which contains savedir and savedirfd, and pass that though
instead.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Dec 6 2018, 5:36 PM

Herald added a subscriber: jonathan. · View Herald TranscriptDec 6 2018, 5:36 PM

Harbormaster completed remote builds in B21402: Diff 51669.Dec 6 2018, 5:36 PM

markj edited the summary of this revision. (Show Details)Dec 6 2018, 5:41 PM

markj added reviewers: oshogbo, emaste, cem, capsicum.

Generally LGTM. Thanks for doing this.

sbin/savecore/savecore.c
109 ↗	(On Diff #51669)	It's weird that libz doesn't provide a header with these interfaces. It's orthogonal to this change, but given you just added `zdopen(3)`, maybe it's worth revisiting?
206–207 ↗	(On Diff #51669)	It seems like something like `fopenat()` might be desirable in a capsicum world. Is that infringing too much on POSIX? Orthogonal to this review. OTOH, if fdopen() fails, we leak `fd`. Right? That's an argument for abstracting this.
235–236 ↗	(On Diff #51669)	ditto — fdopen failure leaks fd
261 ↗	(On Diff #51669)	This seems unnecessarily fragile, (not handling interrupted writes) but is not a regression in this change.
272 ↗	(On Diff #51669)	`savedirfd`, to match convention? Or at least `dirfd`?
360–362 ↗	(On Diff #51669)	`fd` leaked
782–783 ↗	(On Diff #51669)	same fd leak on fdopen failure problem
848–856 ↗	(On Diff #51669)	It seems a like a short (interrupted) `read()` might result in partial key contents remaining in memory. (Not a regression in this patch, just an observation.)
940–943 ↗	(On Diff #51669)	I'd suggest initializing the various fds to -1 and only closing them if non-negative here. (Of course, `close(-1)` is harmless, just looks bad in truss.) (When an `fdopen()` succeeds, the corresponding `fd` should be marked -1 to demonstrate the transfer of ownership.)
1060–1091 ↗	(On Diff #51669)	Could this be moved to a subroutine?
1065 ↗	(On Diff #51669)	8 seems optimistic. What's the most swap devices you've ever seen configured? I have one swap partition myself. I don't feel strongly about it, because it really doesn't matter, but I'd start at more like 2.
1095–1102 ↗	(On Diff #51669)	We should restrict caps on `savedirfd` at some point. If we did somewhere in this revision, I must have missed it. It needs LOOKUP obviously, and several others.

This revision is now accepted and ready to land.Dec 7 2018, 2:40 AM

oshogbo added a reviewer: def.Dec 7 2018, 9:58 AM

oshogbo added inline comments.

sbin/savecore/savecore.c
125 ↗	(On Diff #51669)	Shouldnt we use syslog before enter capability mode and after it just cap_syslog?

cem added inline comments.Dec 7 2018, 4:22 PM

sbin/savecore/savecore.c
125 ↗	(On Diff #51669)	That's what this routine is doing, in a roundabout way.

markj marked 13 inline comments as done.Dec 7 2018, 4:56 PM

markj added a subscriber: delphij.

markj added inline comments.

sbin/savecore/savecore.c
109 ↗	(On Diff #51669)	Yeah, might be worth having a zlib_freebsd.h or so? Or add them to zlib.h but make them opt-in with an ifdef. @delphij have you thought about this at all?
125 ↗	(On Diff #51669)	Maybe? I somewhat prefer to just use logmsg() everywhere. Is there a downside to that approach?
206–207 ↗	(On Diff #51669)	Yes, I would like an fopenat(). Writing openat()+fdopen() is tiresome and error-prone, as you noticed. :(
940–943 ↗	(On Diff #51669)	Note that fddev is never used to create a file stream. I just renamed a couple of variables to make their origin more clear.
1065 ↗	(On Diff #51669)	Yeah, I'm more just thinking about the number of bytes allocated, which is insignificant either way.
1095–1102 ↗	(On Diff #51669)	It's a bit unsatisfying because rights are inherited by the fds opened using savedirfd, so we need the kitchen sink.

Address feedback from cem.

This revision now requires review to proceed.Dec 7 2018, 4:57 PM

Harbormaster completed remote builds in B21432: Diff 51727.Dec 7 2018, 4:58 PM

cem added inline comments.Dec 7 2018, 5:14 PM

sbin/savecore/savecore.c
940–943 ↗	(On Diff #51669)	Sure; I was suggesting adding fdcore and fdinfo here, which are used for streams.
1065 ↗	(On Diff #51669)	Yep — fine as-is.
1095–1102 ↗	(On Diff #51669)	We'd need a lot, but still less than the full unrestricted set.

cem accepted this revision.Dec 7 2018, 5:17 PM

cem added inline comments.

sbin/savecore/savecore.c
208–210 ↗	(On Diff #51727)	Just write an `fopenat`. Even if it's local to this program for now :-). Worst case, `savecore_fopenat()`. The duplication is silly.

This revision is now accepted and ready to land.Dec 7 2018, 5:17 PM

markj marked an inline comment as done.Dec 7 2018, 5:40 PM

markj added inline comments.

sbin/savecore/savecore.c

208–210 ↗

(On Diff #51727)

It's a bit of a monster:

static FILE *                                                                                                                                                                 
xfopenat(int dirfd, const char *path, int flags, ...)                                                                                                                         
{                                                                                                                                                                             
        va_list ap;                                                                                                                                                           
        FILE *fp;                                                                                                                                                             
        const char *modestr;                                                                                                                                                  
        mode_t mode;                                                                                                                                                          
        int error, fd;                                                                                                                                                        
                                                                                                                                                                              
        if ((flags & O_CREAT) != 0) {                                                                                                                                         
                va_start(ap, flags);                                                                                                                                          
                mode = (mode_t)va_arg(ap, int);                                                                                                                               
                va_end(ap);                                                                                                                                                   
        } else                                                                                                                                                                
                mode = 0;                                                                                                                                                     
                                                                                                                                                                              
        if ((flags & O_RDWR) == O_RDWR)                                                                                                                                       
                modestr = "a+";                                                                                                                                               
        else if ((flags & O_WRONLY) == O_WRONLY)                                                                                                                              
                modestr = "a";                                                                                                                                                
        else                                                                                                                                                                  
                modestr = "r";                                                                                                                                                
                                                                                                                                                                              
        fd = openat(dirfd, path, flags, mode);                                                                                                                                
        if (fd < 0)                                                                                                                                                           
                return (NULL);                                                                                                                                                
        fp = fdopen(fd, modestr);                                                                                                                                             
        if (fp == NULL) {                                                                                                                                                     
                error = errno;                                                                                                                                                
                (void)close(fd);                                                                                                                                              
                errno = error;                                                                                                                                                
        }                                                                                                                                                                     
        return (fp);                                                                                                                                                          
}

I'll just make the caller pass the mode string, though I find that kind of silly.

Add xfopenat().

This revision now requires review to proceed.Dec 7 2018, 5:54 PM

Harbormaster completed remote builds in B21433: Diff 51730.Dec 7 2018, 5:55 PM

cem accepted this revision.Dec 7 2018, 6:46 PM

cem added inline comments.

sbin/savecore/savecore.c
128 ↗	(On Diff #51730)	Ah, I was imagining the smaller API which of just fopen(3) plus a `dirfd`. I.e., `fopenat(int dirfd, const char path, const char mode)`. With the usual translation of mode strings like "r" and "w" into open(2) flags and modes (really 0666 & ~umask, I guess). Nothing in this file actually cares about modestrings other than "r" -> O_RDONLY and "w" -> O_WRONLY \| O_CREAT \| O_TRUNC, though they do care about the `mode_t`. The approach you've taken allows more like the full generality of `openat(2)` and `fdopen(3)`, which is obviously more versatile, and particularly significant for times when you don't want the default create mode (0600 core/info files), or other non-default open flags (unused in this program). But it is a more complicated interface. Given it's local to this file, I don't feel the need to bikeshed it. :-)
135–137 ↗	(On Diff #51730)	I was looking for a function attribute to specify this was 'open(2)-like' and needed a mode argument if `O_CREAT` was passed in flags. I didn't find one in our fcntl.h. GCC has this construct, though I don't know if Clang does: if (__builtin_constant_p(oflag) && (oflag & O_CREAT) != 0 && __builtin_va_arg_pack_len () < 1) { error_or_whatever(); } It only works for "inlined" functions, which this probably counts as given it's same-CU (or would, with the inline keyword). Apparently glibc does something like this when running with `-D_FORTIFY_SOURCE=2` ("__open_missing_mode"). They just add small inline shims around open-like functions that perform only this checking, then dispatch to less general open(2)-subimplementations when possible. Definitely out of scope for this revision, but something we should think about adopting (if Clang supports it). The best part is, this stuff predates GPL3 gcc — even the ancient GCC in sparc/ppc/mips land should support it.
208–210 ↗	(On Diff #51727)	FWIW, O_RDWR is more like "r+" than "a+". "a" would be if O_APPEND was included. (Or "w+", if flags includes O_TRUNC and/or O_CREAT.) And I guess O_EXCL implies "x". It quickly gets hairy, and there's no clean invertible function between open flags and modestrings, so having the caller pass it makes some sense to me.

This revision is now accepted and ready to land.Dec 7 2018, 6:46 PM

markj marked an inline comment as done.Dec 7 2018, 10:07 PM

markj added inline comments.

sbin/savecore/savecore.c
128 ↗	(On Diff #51730)	I just didn't want to parse the mode string. For a library implementation, your interface (omitting open(2) flags) makes more sense.

Disable casper in the rescue build.

This revision now requires review to proceed.Dec 8 2018, 6:22 PM

Harbormaster completed remote builds in B21446: Diff 51758.Dec 8 2018, 6:22 PM

I didn't know we built a rescue savecore!

This revision is now accepted and ready to land.Dec 8 2018, 7:35 PM

Sorry for keeping you waiting.

sbin/savecore/savecore.c
621 ↗	(On Diff #51758)	It would be nice but not needed to limit also ioctls. At the other hand it should not be a part of fileargs?
972 ↗	(On Diff #51758)	just an idea not sure if good but maybe you want to optimize that and use a fileargs_cinitnv ? Instead of building argv just build an nvlist?
1140 ↗	(On Diff #51758)	You need to check errno or caph_rights_limit.

cem added inline comments.Dec 12 2018, 9:06 PM

sbin/savecore/savecore.c
972 ↗	(On Diff #51758)	In practice, I suspect performance is a moot consideration — it's rare to have many dump/swap devices, and it would take quite a lot before this solution was even noticeably slow. If it simplifies the code, though, I am all for it.

Handle oshogbo's comments.

This revision now requires review to proceed.Dec 13 2018, 1:12 AM

markj added inline comments.Dec 13 2018, 1:12 AM

sbin/savecore/savecore.c
621 ↗	(On Diff #51758)	Sorry, I don't quite understand the question. I noted the problem with ioctls in a comment in init_caps(). Are you saying that we should limit ioctls here, after calling fileargs_open()?
972 ↗	(On Diff #51758)	I don't feel strongly either way. It would slightly simplify this code, but then the code for the argc > 0 case would be slightly more complicated. I slightly prefer to just keep it as-is, since, as Conrad pointed out, performance is not important here.