Page MenuHomeFreeBSD

Capsicumize savecore(8).
Needs ReviewPublic

Authored by markj on Thu, Dec 6, 5:36 PM.

Details

Reviewers
oshogbo
emaste
cem
def
Group Reviewers
capsicum
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

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 21528
Build 20839: arc lint + arc unit

Event Timeline

markj created this revision.Thu, Dec 6, 5:36 PM
markj edited the summary of this revision. (Show Details)Thu, Dec 6, 5:41 PM
markj added reviewers: oshogbo, emaste, cem, capsicum.
cem accepted this revision.Fri, Dec 7, 2:40 AM

Generally LGTM. Thanks for doing this.

sbin/savecore/savecore.c
109

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?

233–234

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.

260–263

ditto — fdopen failure leaks fd

286

This seems unnecessarily fragile, (not handling interrupted writes) but is not a regression in this change.

297

savedirfd, to match convention? Or at least dirfd?

384–385

fd leaked

805–808

same fd leak on fdopen failure problem

873–881

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.)

965–1049

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.)

1129–1160

Could this be moved to a subroutine?

1134

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.

1151–1158

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.Fri, Dec 7, 2:40 AM
oshogbo added a reviewer: def.Fri, Dec 7, 9:58 AM
oshogbo added inline comments.
sbin/savecore/savecore.c
125

Shouldnt we use syslog before enter capability mode and after it just cap_syslog?

cem added inline comments.Fri, Dec 7, 4:22 PM
sbin/savecore/savecore.c
125

That's what this routine is doing, in a roundabout way.

markj marked 13 inline comments as done.Fri, Dec 7, 4:56 PM
markj added a subscriber: delphij.
markj added inline comments.
sbin/savecore/savecore.c
109

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

Maybe? I somewhat prefer to just use logmsg() everywhere. Is there a downside to that approach?

233–234

Yes, I would like an fopenat(). Writing openat()+fdopen() is tiresome and error-prone, as you noticed. :(

965–1049

Note that fddev is never used to create a file stream. I just renamed a couple of variables to make their origin more clear.

1134

Yeah, I'm more just thinking about the number of bytes allocated, which is insignificant either way.

1151–1158

It's a bit unsatisfying because rights are inherited by the fds opened using savedirfd, so we need the kitchen sink.

markj updated this revision to Diff 51727.Fri, Dec 7, 4:57 PM
markj marked 2 inline comments as done.
  • Address feedback from cem.
This revision now requires review to proceed.Fri, Dec 7, 4:57 PM
cem added inline comments.Fri, Dec 7, 5:14 PM
sbin/savecore/savecore.c
965–1049

Sure; I was suggesting adding fdcore and fdinfo here, which are used for streams.

1134

Yep — fine as-is.

1151–1158

We'd need a lot, but still less than the full unrestricted set.

cem accepted this revision.Fri, Dec 7, 5:17 PM
cem added inline comments.
sbin/savecore/savecore.c
235–237

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.Fri, Dec 7, 5:17 PM
markj marked an inline comment as done.Fri, Dec 7, 5:40 PM
markj added inline comments.
sbin/savecore/savecore.c
235–237

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.

markj updated this revision to Diff 51730.Fri, Dec 7, 5:54 PM
  • Add xfopenat().
This revision now requires review to proceed.Fri, Dec 7, 5:54 PM
cem accepted this revision.Fri, Dec 7, 6:46 PM
cem added inline comments.
sbin/savecore/savecore.c
128

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

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.

235–237

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.Fri, Dec 7, 6:46 PM
markj marked an inline comment as done.Fri, Dec 7, 10:07 PM
markj added inline comments.
sbin/savecore/savecore.c
128

I just didn't want to parse the mode string. For a library implementation, your interface (omitting open(2) flags) makes more sense.

markj updated this revision to Diff 51758.Sat, Dec 8, 6:22 PM
  • Disable casper in the rescue build.
This revision now requires review to proceed.Sat, Dec 8, 6:22 PM
cem accepted this revision.Sat, Dec 8, 7:35 PM

I didn't know we built a rescue savecore!

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

Sorry for keeping you waiting.

sbin/savecore/savecore.c
621–623

It would be nice but not needed to limit also ioctls.
At the other hand it should not be a part of fileargs?

972

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

You need to check errno or caph_rights_limit.

cem added inline comments.Wed, Dec 12, 9:06 PM
sbin/savecore/savecore.c
972

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.

markj updated this revision to Diff 51941.Thu, Dec 13, 1:12 AM
markj marked 3 inline comments as done.
  • Handle oshogbo's comments.
This revision now requires review to proceed.Thu, Dec 13, 1:12 AM
markj added inline comments.Thu, Dec 13, 1:12 AM
sbin/savecore/savecore.c
621–623

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

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.