Page MenuHomeFreeBSD

Capsicumize savecore(8).
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.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.
cem accepted this revision.Dec 7 2018, 2:40 AM

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.

markj updated this revision to Diff 51727.Dec 7 2018, 4:57 PM
markj marked 2 inline comments as done.
  • Address feedback from cem.
This revision now requires review to proceed.Dec 7 2018, 4:57 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.

markj updated this revision to Diff 51730.Dec 7 2018, 5:54 PM
  • Add xfopenat().
This revision now requires review to proceed.Dec 7 2018, 5:54 PM
cem accepted this revision.Dec 7 2018, 6:46 PM
cem added inline comments.
sbin/savecore/savecore.c
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.

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.

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.

markj updated this revision to Diff 51758.Dec 8 2018, 6:22 PM
  • Disable casper in the rescue build.
This revision now requires review to proceed.Dec 8 2018, 6:22 PM
cem accepted this revision.Dec 8 2018, 7:35 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.

markj updated this revision to Diff 51941.Dec 13 2018, 1:12 AM
markj marked 3 inline comments as done.
  • 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.

oshogbo accepted this revision.Jan 2 2019, 4:09 PM
This revision is now accepted and ready to land.Jan 2 2019, 4:09 PM
Closed by commit rS342699: Capsicumize savecore(8). (authored by markj, committed by ). · Explain WhyJan 2 2019, 5:09 PM
This revision was automatically updated to reflect the committed changes.