| Message ID | 20191114024807.2371-1-kongbaichuan@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | restorecond: Fix redundant console log output error | expand |
On 11/13/19 9:48 PM, Baichuan Kong wrote: > When starting restorecond without any option the following redundant > console log is outputed: > > /dev/log 100.0% > /var/volatile/run/syslogd.pid 100.0% > ... > > This is caused by two global variables of same name r_opts. When > executes r_opts = opts in restore_init(), it originally intends > to assign the address of struct r_opts in "restorecond.c" to the > pointer *r_opts in "restore.c". > > However, the address is assigned to the struct r_opts and covers > the value of low eight bytes in it. That causes unexpected value > of member varibale 'nochange' and 'verbose' in struct r_opts, thus > affects value of 'restorecon_flags' and executes unexpected operations > when restorecon the files such as the redundant console log output or > file label nochange. > > Cause restorecond/restore.c is copied from policycoreutils/setfiles, > which share the same pattern. It also has potential risk to generate > same problems, So fix it in case. > > Signed-off-by: Baichuan Kong <kongbaichuan@huawei.com> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > policycoreutils/setfiles/restore.c | 42 +++++++++++++++++--------------------- > restorecond/restore.c | 40 ++++++++++++++++-------------------- > 2 files changed, 37 insertions(+), 45 deletions(-) > > diff --git a/policycoreutils/setfiles/restore.c b/policycoreutils/setfiles/restore.c > index 9dea5656..d3335d1a 100644 > --- a/policycoreutils/setfiles/restore.c > +++ b/policycoreutils/setfiles/restore.c > @@ -17,40 +17,37 @@ > char **exclude_list; > int exclude_count; > > -struct restore_opts *r_opts; > - > void restore_init(struct restore_opts *opts) > { > int rc; > > - r_opts = opts; > struct selinux_opt selinux_opts[] = { > - { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate }, > - { SELABEL_OPT_PATH, r_opts->selabel_opt_path }, > - { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest } > + { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate }, > + { SELABEL_OPT_PATH, opts->selabel_opt_path }, > + { SELABEL_OPT_DIGEST, opts->selabel_opt_digest } > }; > > - r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); > - if (!r_opts->hnd) { > - perror(r_opts->selabel_opt_path); > + opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); > + if (!opts->hnd) { > + perror(opts->selabel_opt_path); > exit(1); > } > > - r_opts->restorecon_flags = 0; > - r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose | > - r_opts->progress | r_opts->set_specctx | > - r_opts->add_assoc | r_opts->ignore_digest | > - r_opts->recurse | r_opts->userealpath | > - r_opts->xdev | r_opts->abort_on_error | > - r_opts->syslog_changes | r_opts->log_matches | > - r_opts->ignore_noent | r_opts->ignore_mounts | > - r_opts->mass_relabel; > + opts->restorecon_flags = 0; > + opts->restorecon_flags = opts->nochange | opts->verbose | > + opts->progress | opts->set_specctx | > + opts->add_assoc | opts->ignore_digest | > + opts->recurse | opts->userealpath | > + opts->xdev | opts->abort_on_error | > + opts->syslog_changes | opts->log_matches | > + opts->ignore_noent | opts->ignore_mounts | > + opts->mass_relabel; > > /* Use setfiles, restorecon and restorecond own handles */ > - selinux_restorecon_set_sehandle(r_opts->hnd); > + selinux_restorecon_set_sehandle(opts->hnd); > > - if (r_opts->rootpath) { > - rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath); > + if (opts->rootpath) { > + rc = selinux_restorecon_set_alt_rootpath(opts->rootpath); > if (rc) { > fprintf(stderr, > "selinux_restorecon_set_alt_rootpath error: %s.\n", > @@ -81,7 +78,6 @@ int process_glob(char *name, struct restore_opts *opts) > size_t i = 0; > int len, rc, errors; > > - r_opts = opts; > memset(&globbuf, 0, sizeof(globbuf)); > > errors = glob(name, GLOB_TILDE | GLOB_PERIOD | > @@ -96,7 +92,7 @@ int process_glob(char *name, struct restore_opts *opts) > if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0) > continue; > rc = selinux_restorecon(globbuf.gl_pathv[i], > - r_opts->restorecon_flags); > + opts->restorecon_flags); > if (rc < 0) > errors = rc; > } > diff --git a/restorecond/restore.c b/restorecond/restore.c > index f6e30001..b93b5fdb 100644 > --- a/restorecond/restore.c > +++ b/restorecond/restore.c > @@ -12,39 +12,36 @@ > char **exclude_list; > int exclude_count; > > -struct restore_opts *r_opts; > - > void restore_init(struct restore_opts *opts) > { > int rc; > > - r_opts = opts; > struct selinux_opt selinux_opts[] = { > - { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate }, > - { SELABEL_OPT_PATH, r_opts->selabel_opt_path }, > - { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest } > + { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate }, > + { SELABEL_OPT_PATH, opts->selabel_opt_path }, > + { SELABEL_OPT_DIGEST, opts->selabel_opt_digest } > }; > > - r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); > - if (!r_opts->hnd) { > - perror(r_opts->selabel_opt_path); > + opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); > + if (!opts->hnd) { > + perror(opts->selabel_opt_path); > exit(1); > } > > - r_opts->restorecon_flags = 0; > - r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose | > - r_opts->progress | r_opts->set_specctx | > - r_opts->add_assoc | r_opts->ignore_digest | > - r_opts->recurse | r_opts->userealpath | > - r_opts->xdev | r_opts->abort_on_error | > - r_opts->syslog_changes | r_opts->log_matches | > - r_opts->ignore_noent | r_opts->ignore_mounts; > + opts->restorecon_flags = 0; > + opts->restorecon_flags = opts->nochange | opts->verbose | > + opts->progress | opts->set_specctx | > + opts->add_assoc | opts->ignore_digest | > + opts->recurse | opts->userealpath | > + opts->xdev | opts->abort_on_error | > + opts->syslog_changes | opts->log_matches | > + opts->ignore_noent | opts->ignore_mounts; > > /* Use setfiles, restorecon and restorecond own handles */ > - selinux_restorecon_set_sehandle(r_opts->hnd); > + selinux_restorecon_set_sehandle(opts->hnd); > > - if (r_opts->rootpath) { > - rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath); > + if (opts->rootpath) { > + rc = selinux_restorecon_set_alt_rootpath(opts->rootpath); > if (rc) { > fprintf(stderr, > "selinux_restorecon_set_alt_rootpath error: %s.\n", > @@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts) > size_t i = 0; > int len, rc, errors; > > - r_opts = opts; > memset(&globbuf, 0, sizeof(globbuf)); > > errors = glob(name, GLOB_TILDE | GLOB_PERIOD | > @@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts) > if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0) > continue; > rc = selinux_restorecon(globbuf.gl_pathv[i], > - r_opts->restorecon_flags); > + opts->restorecon_flags); > if (rc < 0) > errors = rc; > } >
On 11/14/19 10:18 AM, Stephen Smalley wrote: > On 11/13/19 9:48 PM, Baichuan Kong wrote: >> When starting restorecond without any option the following redundant >> console log is outputed: >> >> /dev/log 100.0% >> /var/volatile/run/syslogd.pid 100.0% >> ... >> >> This is caused by two global variables of same name r_opts. When >> executes r_opts = opts in restore_init(), it originally intends >> to assign the address of struct r_opts in "restorecond.c" to the >> pointer *r_opts in "restore.c". >> >> However, the address is assigned to the struct r_opts and covers >> the value of low eight bytes in it. That causes unexpected value >> of member varibale 'nochange' and 'verbose' in struct r_opts, thus >> affects value of 'restorecon_flags' and executes unexpected operations >> when restorecon the files such as the redundant console log output or >> file label nochange. >> >> Cause restorecond/restore.c is copied from policycoreutils/setfiles, >> which share the same pattern. It also has potential risk to generate >> same problems, So fix it in case. >> >> Signed-off-by: Baichuan Kong <kongbaichuan@huawei.com> > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> Thanks, applied. > >> --- >> policycoreutils/setfiles/restore.c | 42 >> +++++++++++++++++--------------------- >> restorecond/restore.c | 40 >> ++++++++++++++++-------------------- >> 2 files changed, 37 insertions(+), 45 deletions(-) >> >> diff --git a/policycoreutils/setfiles/restore.c >> b/policycoreutils/setfiles/restore.c >> index 9dea5656..d3335d1a 100644 >> --- a/policycoreutils/setfiles/restore.c >> +++ b/policycoreutils/setfiles/restore.c >> @@ -17,40 +17,37 @@ >> char **exclude_list; >> int exclude_count; >> -struct restore_opts *r_opts; >> - >> void restore_init(struct restore_opts *opts) >> { >> int rc; >> - r_opts = opts; >> struct selinux_opt selinux_opts[] = { >> - { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate }, >> - { SELABEL_OPT_PATH, r_opts->selabel_opt_path }, >> - { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest } >> + { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate }, >> + { SELABEL_OPT_PATH, opts->selabel_opt_path }, >> + { SELABEL_OPT_DIGEST, opts->selabel_opt_digest } >> }; >> - r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); >> - if (!r_opts->hnd) { >> - perror(r_opts->selabel_opt_path); >> + opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); >> + if (!opts->hnd) { >> + perror(opts->selabel_opt_path); >> exit(1); >> } >> - r_opts->restorecon_flags = 0; >> - r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose | >> - r_opts->progress | r_opts->set_specctx | >> - r_opts->add_assoc | r_opts->ignore_digest | >> - r_opts->recurse | r_opts->userealpath | >> - r_opts->xdev | r_opts->abort_on_error | >> - r_opts->syslog_changes | r_opts->log_matches | >> - r_opts->ignore_noent | r_opts->ignore_mounts | >> - r_opts->mass_relabel; >> + opts->restorecon_flags = 0; >> + opts->restorecon_flags = opts->nochange | opts->verbose | >> + opts->progress | opts->set_specctx | >> + opts->add_assoc | opts->ignore_digest | >> + opts->recurse | opts->userealpath | >> + opts->xdev | opts->abort_on_error | >> + opts->syslog_changes | opts->log_matches | >> + opts->ignore_noent | opts->ignore_mounts | >> + opts->mass_relabel; >> /* Use setfiles, restorecon and restorecond own handles */ >> - selinux_restorecon_set_sehandle(r_opts->hnd); >> + selinux_restorecon_set_sehandle(opts->hnd); >> - if (r_opts->rootpath) { >> - rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath); >> + if (opts->rootpath) { >> + rc = selinux_restorecon_set_alt_rootpath(opts->rootpath); >> if (rc) { >> fprintf(stderr, >> "selinux_restorecon_set_alt_rootpath error: %s.\n", >> @@ -81,7 +78,6 @@ int process_glob(char *name, struct restore_opts *opts) >> size_t i = 0; >> int len, rc, errors; >> - r_opts = opts; >> memset(&globbuf, 0, sizeof(globbuf)); >> errors = glob(name, GLOB_TILDE | GLOB_PERIOD | >> @@ -96,7 +92,7 @@ int process_glob(char *name, struct restore_opts *opts) >> if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0) >> continue; >> rc = selinux_restorecon(globbuf.gl_pathv[i], >> - r_opts->restorecon_flags); >> + opts->restorecon_flags); >> if (rc < 0) >> errors = rc; >> } >> diff --git a/restorecond/restore.c b/restorecond/restore.c >> index f6e30001..b93b5fdb 100644 >> --- a/restorecond/restore.c >> +++ b/restorecond/restore.c >> @@ -12,39 +12,36 @@ >> char **exclude_list; >> int exclude_count; >> -struct restore_opts *r_opts; >> - >> void restore_init(struct restore_opts *opts) >> { >> int rc; >> - r_opts = opts; >> struct selinux_opt selinux_opts[] = { >> - { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate }, >> - { SELABEL_OPT_PATH, r_opts->selabel_opt_path }, >> - { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest } >> + { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate }, >> + { SELABEL_OPT_PATH, opts->selabel_opt_path }, >> + { SELABEL_OPT_DIGEST, opts->selabel_opt_digest } >> }; >> - r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); >> - if (!r_opts->hnd) { >> - perror(r_opts->selabel_opt_path); >> + opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); >> + if (!opts->hnd) { >> + perror(opts->selabel_opt_path); >> exit(1); >> } >> - r_opts->restorecon_flags = 0; >> - r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose | >> - r_opts->progress | r_opts->set_specctx | >> - r_opts->add_assoc | r_opts->ignore_digest | >> - r_opts->recurse | r_opts->userealpath | >> - r_opts->xdev | r_opts->abort_on_error | >> - r_opts->syslog_changes | r_opts->log_matches | >> - r_opts->ignore_noent | r_opts->ignore_mounts; >> + opts->restorecon_flags = 0; >> + opts->restorecon_flags = opts->nochange | opts->verbose | >> + opts->progress | opts->set_specctx | >> + opts->add_assoc | opts->ignore_digest | >> + opts->recurse | opts->userealpath | >> + opts->xdev | opts->abort_on_error | >> + opts->syslog_changes | opts->log_matches | >> + opts->ignore_noent | opts->ignore_mounts; >> /* Use setfiles, restorecon and restorecond own handles */ >> - selinux_restorecon_set_sehandle(r_opts->hnd); >> + selinux_restorecon_set_sehandle(opts->hnd); >> - if (r_opts->rootpath) { >> - rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath); >> + if (opts->rootpath) { >> + rc = selinux_restorecon_set_alt_rootpath(opts->rootpath); >> if (rc) { >> fprintf(stderr, >> "selinux_restorecon_set_alt_rootpath error: %s.\n", >> @@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts) >> size_t i = 0; >> int len, rc, errors; >> - r_opts = opts; >> memset(&globbuf, 0, sizeof(globbuf)); >> errors = glob(name, GLOB_TILDE | GLOB_PERIOD | >> @@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts) >> if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0) >> continue; >> rc = selinux_restorecon(globbuf.gl_pathv[i], >> - r_opts->restorecon_flags); >> + opts->restorecon_flags); >> if (rc < 0) >> errors = rc; >> } >> >
diff --git a/policycoreutils/setfiles/restore.c b/policycoreutils/setfiles/restore.c index 9dea5656..d3335d1a 100644 --- a/policycoreutils/setfiles/restore.c +++ b/policycoreutils/setfiles/restore.c @@ -17,40 +17,37 @@ char **exclude_list; int exclude_count; -struct restore_opts *r_opts; - void restore_init(struct restore_opts *opts) { int rc; - r_opts = opts; struct selinux_opt selinux_opts[] = { - { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate }, - { SELABEL_OPT_PATH, r_opts->selabel_opt_path }, - { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest } + { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate }, + { SELABEL_OPT_PATH, opts->selabel_opt_path }, + { SELABEL_OPT_DIGEST, opts->selabel_opt_digest } }; - r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); - if (!r_opts->hnd) { - perror(r_opts->selabel_opt_path); + opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); + if (!opts->hnd) { + perror(opts->selabel_opt_path); exit(1); } - r_opts->restorecon_flags = 0; - r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose | - r_opts->progress | r_opts->set_specctx | - r_opts->add_assoc | r_opts->ignore_digest | - r_opts->recurse | r_opts->userealpath | - r_opts->xdev | r_opts->abort_on_error | - r_opts->syslog_changes | r_opts->log_matches | - r_opts->ignore_noent | r_opts->ignore_mounts | - r_opts->mass_relabel; + opts->restorecon_flags = 0; + opts->restorecon_flags = opts->nochange | opts->verbose | + opts->progress | opts->set_specctx | + opts->add_assoc | opts->ignore_digest | + opts->recurse | opts->userealpath | + opts->xdev | opts->abort_on_error | + opts->syslog_changes | opts->log_matches | + opts->ignore_noent | opts->ignore_mounts | + opts->mass_relabel; /* Use setfiles, restorecon and restorecond own handles */ - selinux_restorecon_set_sehandle(r_opts->hnd); + selinux_restorecon_set_sehandle(opts->hnd); - if (r_opts->rootpath) { - rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath); + if (opts->rootpath) { + rc = selinux_restorecon_set_alt_rootpath(opts->rootpath); if (rc) { fprintf(stderr, "selinux_restorecon_set_alt_rootpath error: %s.\n", @@ -81,7 +78,6 @@ int process_glob(char *name, struct restore_opts *opts) size_t i = 0; int len, rc, errors; - r_opts = opts; memset(&globbuf, 0, sizeof(globbuf)); errors = glob(name, GLOB_TILDE | GLOB_PERIOD | @@ -96,7 +92,7 @@ int process_glob(char *name, struct restore_opts *opts) if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0) continue; rc = selinux_restorecon(globbuf.gl_pathv[i], - r_opts->restorecon_flags); + opts->restorecon_flags); if (rc < 0) errors = rc; } diff --git a/restorecond/restore.c b/restorecond/restore.c index f6e30001..b93b5fdb 100644 --- a/restorecond/restore.c +++ b/restorecond/restore.c @@ -12,39 +12,36 @@ char **exclude_list; int exclude_count; -struct restore_opts *r_opts; - void restore_init(struct restore_opts *opts) { int rc; - r_opts = opts; struct selinux_opt selinux_opts[] = { - { SELABEL_OPT_VALIDATE, r_opts->selabel_opt_validate }, - { SELABEL_OPT_PATH, r_opts->selabel_opt_path }, - { SELABEL_OPT_DIGEST, r_opts->selabel_opt_digest } + { SELABEL_OPT_VALIDATE, opts->selabel_opt_validate }, + { SELABEL_OPT_PATH, opts->selabel_opt_path }, + { SELABEL_OPT_DIGEST, opts->selabel_opt_digest } }; - r_opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); - if (!r_opts->hnd) { - perror(r_opts->selabel_opt_path); + opts->hnd = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3); + if (!opts->hnd) { + perror(opts->selabel_opt_path); exit(1); } - r_opts->restorecon_flags = 0; - r_opts->restorecon_flags = r_opts->nochange | r_opts->verbose | - r_opts->progress | r_opts->set_specctx | - r_opts->add_assoc | r_opts->ignore_digest | - r_opts->recurse | r_opts->userealpath | - r_opts->xdev | r_opts->abort_on_error | - r_opts->syslog_changes | r_opts->log_matches | - r_opts->ignore_noent | r_opts->ignore_mounts; + opts->restorecon_flags = 0; + opts->restorecon_flags = opts->nochange | opts->verbose | + opts->progress | opts->set_specctx | + opts->add_assoc | opts->ignore_digest | + opts->recurse | opts->userealpath | + opts->xdev | opts->abort_on_error | + opts->syslog_changes | opts->log_matches | + opts->ignore_noent | opts->ignore_mounts; /* Use setfiles, restorecon and restorecond own handles */ - selinux_restorecon_set_sehandle(r_opts->hnd); + selinux_restorecon_set_sehandle(opts->hnd); - if (r_opts->rootpath) { - rc = selinux_restorecon_set_alt_rootpath(r_opts->rootpath); + if (opts->rootpath) { + rc = selinux_restorecon_set_alt_rootpath(opts->rootpath); if (rc) { fprintf(stderr, "selinux_restorecon_set_alt_rootpath error: %s.\n", @@ -75,7 +72,6 @@ int process_glob(char *name, struct restore_opts *opts) size_t i = 0; int len, rc, errors; - r_opts = opts; memset(&globbuf, 0, sizeof(globbuf)); errors = glob(name, GLOB_TILDE | GLOB_PERIOD | @@ -90,7 +86,7 @@ int process_glob(char *name, struct restore_opts *opts) if (len > 0 && strcmp(&globbuf.gl_pathv[i][len], "/..") == 0) continue; rc = selinux_restorecon(globbuf.gl_pathv[i], - r_opts->restorecon_flags); + opts->restorecon_flags); if (rc < 0) errors = rc; }
When starting restorecond without any option the following redundant console log is outputed: /dev/log 100.0% /var/volatile/run/syslogd.pid 100.0% ... This is caused by two global variables of same name r_opts. When executes r_opts = opts in restore_init(), it originally intends to assign the address of struct r_opts in "restorecond.c" to the pointer *r_opts in "restore.c". However, the address is assigned to the struct r_opts and covers the value of low eight bytes in it. That causes unexpected value of member varibale 'nochange' and 'verbose' in struct r_opts, thus affects value of 'restorecon_flags' and executes unexpected operations when restorecon the files such as the redundant console log output or file label nochange. Cause restorecond/restore.c is copied from policycoreutils/setfiles, which share the same pattern. It also has potential risk to generate same problems, So fix it in case. Signed-off-by: Baichuan Kong <kongbaichuan@huawei.com> --- policycoreutils/setfiles/restore.c | 42 +++++++++++++++++--------------------- restorecond/restore.c | 40 ++++++++++++++++-------------------- 2 files changed, 37 insertions(+), 45 deletions(-)