Page MenuHomeFreeBSD
Differential D31453

netdump: send key before dump, in case dump fails
ClosedPublic
Actions

Authored by vangyzen on Aug 7 2021, 2:10 PM.
Tags
None
Referenced Files
F115073102: D31453.id93497.diff
Sun, Apr 20, 5:19 AM
Unknown Object (File)
Sun, Apr 20, 12:30 AM
Unknown Object (File)
Sun, Apr 20, 12:25 AM
Unknown Object (File)
Mar 18 2025, 3:07 PM
Unknown Object (File)
Mar 4 2025, 8:00 PM
Unknown Object (File)
Feb 5 2025, 2:26 PM
Unknown Object (File)
Feb 5 2025, 3:20 AM
Unknown Object (File)
Jan 22 2025, 3:57 PM
View All 90 Files
Subscribers
badger
dab
glebius
imp
melifaro

Details

Reviewers
bdrewery
markj
Commits
rG8320036255f0: netdump: send key before dump, in case dump fails
rG13a58148de17: netdump: send key before dump, in case dump fails
Summary

Previously, if an encrypted netdump failed, such as due to a timeout or
network failure, the key was not saved, so a partial dump was
completely useless.

Send the key first, so the partial dump can be decrypted, because even a
partial dump can be useful.

Test Plan

I ran ls in a loop during a netdump and watched netdumpd create the
key file first. Decryption was successful.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 40971
Build 37860: arc lint + arc unit

Event Timeline

vangyzen created this revision.Aug 7 2021, 2:10 PM
Herald added subscribers: melifaro, dab, badger, imp. · View Herald TranscriptAug 7 2021, 2:10 PM
vangyzen requested review of this revision.Aug 7 2021, 2:10 PM
Harbormaster completed remote builds in B40917: Diff 93371.Aug 7 2021, 2:10 PM
markj added inline comments.Aug 9 2021, 8:42 PM
sys/kern/kern_shutdown.c
1504

How can this situation arise? keysize will be 0 if kdc is NULL.

sys/netinet/netdump/netdump_client.c
352

Do you need a netdump_cleanup() call in the error paths?

bdrewery added inline comments.Aug 9 2021, 11:01 PM
sys/netinet/netdump/netdump_client.c
352

Yup, since line 339 is setting the global pcb.

vangyzen updated this revision to Diff 93497.Aug 10 2021, 5:20 PM
vangyzen marked 2 inline comments as done.
  • CR feedback: call cleanup on error paths
Harbormaster completed remote builds in B40971: Diff 93497.Aug 10 2021, 5:20 PM
vangyzen marked an inline comment as done.Aug 10 2021, 5:21 PM
vangyzen added inline comments.
sys/kern/kern_shutdown.c
1504

This avoids a NULL dereference. In the old code, we dereferenced kdc without testing for NULL. Unfortunately, I don't have more detail, since I made this change locally six months ago, failed to explain this bit in the internal code review, and forgot to upstream it right away. It _seems_ like we should hit it easily. Maybe clang is reordering things enough to accidentally protect us?

sys/netinet/netdump/netdump_client.c
352

Good catch. Thanks.

vangyzen marked an inline comment as done.Aug 10 2021, 5:26 PM
vangyzen added inline comments.
sys/kern/kern_shutdown.c
1504

Or maybe I made this change purely for symmetry with dump_start, and the NULL dereference would have happened in that function without this extra guard?

markj added inline comments.Aug 10 2021, 5:45 PM
sys/kern/kern_shutdown.c
1504

I see. kdc->kdc_dumpkey will just evaluate to (char *)kdc + offsetof(struct kerneldumpcrypto, kdc_dumpkey) because kdc_dumpkey is a VLA, so no dereference happens. Do you have some local changes which invalidate that?

vangyzen marked an inline comment as done.Aug 10 2021, 11:41 PM
vangyzen added inline comments.
sys/kern/kern_shutdown.c
1504

I see. Thanks. I obviously didn't look that closely at the type. My bad.

As it turns out, key is no longer used in this function, so I'll just remove it.

vangyzen updated this revision to Diff 93510.Aug 11 2021, 12:09 AM
vangyzen marked an inline comment as done.
  • CR feedback: remove unused "key" local var
Harbormaster completed remote builds in B40976: Diff 93510.Aug 11 2021, 12:09 AM
markj accepted this revision.Aug 11 2021, 1:48 PM
This revision is now accepted and ready to land.Aug 11 2021, 1:48 PM
Closed by commit rG13a58148de17: netdump: send key before dump, in case dump fails (authored by vangyzen). · Explain WhyAug 11 2021, 8:55 PM
This revision was automatically updated to reflect the committed changes.
vangyzen added a commit: rG13a58148de17: netdump: send key before dump, in case dump fails.
mhorne added a commit: rG8320036255f0: netdump: send key before dump, in case dump fails.Jun 27 2022, 7:35 PM
Herald added a subscriber: glebius. · View Herald TranscriptJun 27 2022, 7:35 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
20 lines
netinet/
netdump/
42 lines
sys/
5 lines

Diff 93497

View Options

sys/kern/kern_shutdown.c

Show First 20 Lines • Show All 56 Lines • ▼ Show 20 Lines
int env_verbosity; int env_verbosity;
static void usage(void); static void usage(void);
int int
main(int argc, char **argv) main(int argc, char **argv)
{ {
char *altpath, **ep, *p, **parg; char *altpath, **ep, *p, **parg, term;
char *cleanenv[1]; char *cleanenv[1];
int ch, want_clear; int ch, want_clear;
int rtrn; int rtrn;
altpath = NULL; altpath = NULL;
want_clear = 0; want_clear = 0;
while ((ch = getopt(argc, argv, "-iP:S:u:v")) != -1) term = '\n';
while ((ch = getopt(argc, argv, "-0iP:S:u:v")) != -1)
switch(ch) { switch(ch) {
case '-': case '-':
case 'i': case 'i':
want_clear = 1; want_clear = 1;
break; break;
case '0':
term = '\0';
break;
case 'P': case 'P':
altpath = strdup(optarg); altpath = strdup(optarg);
break; break;
case 'S': case 'S':
/* /*
* The -S option, for "split string on spaces, with * The -S option, for "split string on spaces, with
* support for some simple substitutions"... * support for some simple substitutions"...
*/ */
Show All 27 Lines if (env_verbosity)
fprintf(stderr, "#env setenv:\t%s\n", *argv); fprintf(stderr, "#env setenv:\t%s\n", *argv);
*p = '\0'; *p = '\0';
rtrn = setenv(*argv, p + 1, 1); rtrn = setenv(*argv, p + 1, 1);
*p = '='; *p = '=';
if (rtrn == -1) if (rtrn == -1)
err(EXIT_FAILURE, "setenv %s", *argv); err(EXIT_FAILURE, "setenv %s", *argv);
} }
if (*argv) { if (*argv) {
if (term == '\0')
errx(125, "cannot specify command with -0");
swillsUnsubmitted
Not Done
Inline Actions

I think maybe this should call exit(), perhaps with a non-zero exit, perhaps with 125 instead.

swills: I think maybe this should call `exit()`, perhaps with a non-zero exit, perhaps with 125 instead.
impUnsubmitted
Not Done
Inline Actions

what's a 125?

imp: what's a 125?
else {
if (altpath) if (altpath)
search_paths(altpath, argv); search_paths(altpath, argv);
if (env_verbosity) { if (env_verbosity) {
fprintf(stderr, "#env executing:\t%s\n", *argv); fprintf(stderr, "#env executing:\t%s\n", *argv);
for (parg = argv, argc = 0; *parg; parg++, argc++) for (parg = argv, argc = 0; *parg; parg++, argc++)
fprintf(stderr, "#env arg[%d]=\t'%s'\n", fprintf(stderr, "#env arg[%d]=\t'%s'\n",
argc, *parg); argc, *parg);
if (env_verbosity > 1) if (env_verbosity > 1)
sleep(1); sleep(1);
} }
execvp(*argv, argv); execvp(*argv, argv);
err(errno == ENOENT ? 127 : 126, "%s", *argv); err(errno == ENOENT ? 127 : 126, "%s", *argv);
} }
}
for (ep = environ; *ep; ep++) for (ep = environ; *ep; ep++)
(void)printf("%s\n", *ep); (void)printf("%s%c", *ep, term);
exit(0); exit(0);
} }
static void static void
usage(void) usage(void)
{ {
(void)fprintf(stderr, (void)fprintf(stderr,
"usage: env [-iv] [-P utilpath] [-S string] [-u name]\n" "usage: env [-0iv] [-P utilpath] [-S string] [-u name]\n"
" [name=value ...] [utility [argument ...]]\n"); " [name=value ...] [utility [argument ...]]\n");
exit(1); exit(1);
} }
View Options

sys/netinet/netdump/netdump_client.c

Show First 20 Lines • Show All 56 Lines • ▼ Show 20 Lines
int env_verbosity; int env_verbosity;
static void usage(void); static void usage(void);
int int
main(int argc, char **argv) main(int argc, char **argv)
{ {
char *altpath, **ep, *p, **parg; char *altpath, **ep, *p, **parg, term;
char *cleanenv[1]; char *cleanenv[1];
int ch, want_clear; int ch, want_clear;
int rtrn; int rtrn;
altpath = NULL; altpath = NULL;
want_clear = 0; want_clear = 0;
while ((ch = getopt(argc, argv, "-iP:S:u:v")) != -1) term = '\n';
while ((ch = getopt(argc, argv, "-0iP:S:u:v")) != -1)
switch(ch) { switch(ch) {
case '-': case '-':
case 'i': case 'i':
want_clear = 1; want_clear = 1;
break; break;
case '0':
term = '\0';
break;
case 'P': case 'P':
altpath = strdup(optarg); altpath = strdup(optarg);
break; break;
case 'S': case 'S':
/* /*
* The -S option, for "split string on spaces, with * The -S option, for "split string on spaces, with
* support for some simple substitutions"... * support for some simple substitutions"...
*/ */
Show All 27 Lines if (env_verbosity)
fprintf(stderr, "#env setenv:\t%s\n", *argv); fprintf(stderr, "#env setenv:\t%s\n", *argv);
*p = '\0'; *p = '\0';
rtrn = setenv(*argv, p + 1, 1); rtrn = setenv(*argv, p + 1, 1);
*p = '='; *p = '=';
if (rtrn == -1) if (rtrn == -1)
err(EXIT_FAILURE, "setenv %s", *argv); err(EXIT_FAILURE, "setenv %s", *argv);
} }
if (*argv) { if (*argv) {
if (term == '\0')
errx(125, "cannot specify command with -0");
swillsUnsubmitted
Not Done
Inline Actions

I think maybe this should call exit(), perhaps with a non-zero exit, perhaps with 125 instead.

swills: I think maybe this should call `exit()`, perhaps with a non-zero exit, perhaps with 125 instead.
impUnsubmitted
Not Done
Inline Actions

what's a 125?

imp: what's a 125?
else {
if (altpath) if (altpath)
search_paths(altpath, argv); search_paths(altpath, argv);
if (env_verbosity) { if (env_verbosity) {
fprintf(stderr, "#env executing:\t%s\n", *argv); fprintf(stderr, "#env executing:\t%s\n", *argv);
for (parg = argv, argc = 0; *parg; parg++, argc++) for (parg = argv, argc = 0; *parg; parg++, argc++)
fprintf(stderr, "#env arg[%d]=\t'%s'\n", fprintf(stderr, "#env arg[%d]=\t'%s'\n",
argc, *parg); argc, *parg);
if (env_verbosity > 1) if (env_verbosity > 1)
sleep(1); sleep(1);
} }
execvp(*argv, argv); execvp(*argv, argv);
err(errno == ENOENT ? 127 : 126, "%s", *argv); err(errno == ENOENT ? 127 : 126, "%s", *argv);
} }
}
for (ep = environ; *ep; ep++) for (ep = environ; *ep; ep++)
(void)printf("%s\n", *ep); (void)printf("%s%c", *ep, term);
exit(0); exit(0);
} }
static void static void
usage(void) usage(void)
{ {
(void)fprintf(stderr, (void)fprintf(stderr,
"usage: env [-iv] [-P utilpath] [-S string] [-u name]\n" "usage: env [-0iv] [-P utilpath] [-S string] [-u name]\n"
" [name=value ...] [utility [argument ...]]\n"); " [name=value ...] [utility [argument ...]]\n");
exit(1); exit(1);
} }
View Options

sys/sys/conf.h

Show First 20 Lines • Show All 56 Lines • ▼ Show 20 Lines
int env_verbosity; int env_verbosity;
static void usage(void); static void usage(void);
int int
main(int argc, char **argv) main(int argc, char **argv)
{ {
char *altpath, **ep, *p, **parg; char *altpath, **ep, *p, **parg, term;
char *cleanenv[1]; char *cleanenv[1];
int ch, want_clear; int ch, want_clear;
int rtrn; int rtrn;
altpath = NULL; altpath = NULL;
want_clear = 0; want_clear = 0;
while ((ch = getopt(argc, argv, "-iP:S:u:v")) != -1) term = '\n';
while ((ch = getopt(argc, argv, "-0iP:S:u:v")) != -1)
switch(ch) { switch(ch) {
case '-': case '-':
case 'i': case 'i':
want_clear = 1; want_clear = 1;
break; break;
case '0':
term = '\0';
break;
case 'P': case 'P':
altpath = strdup(optarg); altpath = strdup(optarg);
break; break;
case 'S': case 'S':
/* /*
* The -S option, for "split string on spaces, with * The -S option, for "split string on spaces, with
* support for some simple substitutions"... * support for some simple substitutions"...
*/ */
Show All 27 Lines if (env_verbosity)
fprintf(stderr, "#env setenv:\t%s\n", *argv); fprintf(stderr, "#env setenv:\t%s\n", *argv);
*p = '\0'; *p = '\0';
rtrn = setenv(*argv, p + 1, 1); rtrn = setenv(*argv, p + 1, 1);
*p = '='; *p = '=';
if (rtrn == -1) if (rtrn == -1)
err(EXIT_FAILURE, "setenv %s", *argv); err(EXIT_FAILURE, "setenv %s", *argv);
} }
if (*argv) { if (*argv) {
if (term == '\0')
errx(125, "cannot specify command with -0");
swillsUnsubmitted
Not Done
Inline Actions

I think maybe this should call exit(), perhaps with a non-zero exit, perhaps with 125 instead.

swills: I think maybe this should call `exit()`, perhaps with a non-zero exit, perhaps with 125 instead.
impUnsubmitted
Not Done
Inline Actions

what's a 125?

imp: what's a 125?
else {
if (altpath) if (altpath)
search_paths(altpath, argv); search_paths(altpath, argv);
if (env_verbosity) { if (env_verbosity) {
fprintf(stderr, "#env executing:\t%s\n", *argv); fprintf(stderr, "#env executing:\t%s\n", *argv);
for (parg = argv, argc = 0; *parg; parg++, argc++) for (parg = argv, argc = 0; *parg; parg++, argc++)
fprintf(stderr, "#env arg[%d]=\t'%s'\n", fprintf(stderr, "#env arg[%d]=\t'%s'\n",
argc, *parg); argc, *parg);
if (env_verbosity > 1) if (env_verbosity > 1)
sleep(1); sleep(1);
} }
execvp(*argv, argv); execvp(*argv, argv);
err(errno == ENOENT ? 127 : 126, "%s", *argv); err(errno == ENOENT ? 127 : 126, "%s", *argv);
} }
}
for (ep = environ; *ep; ep++) for (ep = environ; *ep; ep++)
(void)printf("%s\n", *ep); (void)printf("%s%c", *ep, term);
exit(0); exit(0);
} }
static void static void
usage(void) usage(void)
{ {
(void)fprintf(stderr, (void)fprintf(stderr,
"usage: env [-iv] [-P utilpath] [-S string] [-u name]\n" "usage: env [-0iv] [-P utilpath] [-S string] [-u name]\n"
" [name=value ...] [utility [argument ...]]\n"); " [name=value ...] [utility [argument ...]]\n");
exit(1); exit(1);
} }