On Thu, May 14, 2020 at 08:44:42PM +0200, Nicolas Iooss wrote: > On Wed, May 13, 2020 at 9:34 PM Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > > > > From: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> > > > > Prior to rename(2)'ing the final selinux policy files into place, > > fsync(2) them to ensure the contents will be fully written prior to > > rename. While we are here, also fix checking of write(2) to detect > > short writes and treat them as an error. This code could be more > > generally improved but keeping to the minimal changes required to fix > > this bug. > > > > Fixes: https://github.com/SELinuxProject/selinux/issues/237 > > Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> > > --- > > v2 falls back to EIO if errno is not set by a short write, and > > only fsync's the final selinux policy files being created in > > /etc/selinux, avoiding the overhead of fsync on every file copied > > under /var/lib/selinux. > > > > libsemanage/src/direct_api.c | 10 +++++----- > > libsemanage/src/semanage_store.c | 20 +++++++++++++++----- > > libsemanage/src/semanage_store.h | 4 +++- > > 3 files changed, 23 insertions(+), 11 deletions(-) > > > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c > > index 1088a0ac..d2b91fb2 100644 > > --- a/libsemanage/src/direct_api.c > > +++ b/libsemanage/src/direct_api.c > > @@ -1188,7 +1188,7 @@ cleanup: > > * overwrite it. If source doesn't exist then return success. > > * Returns 0 on success, -1 on error. */ > > static int copy_file_if_exists(const char *src, const char *dst, mode_t mode){ > > - int rc = semanage_copy_file(src, dst, mode); > > + int rc = semanage_copy_file(src, dst, mode, false); > > return (rc < 0 && errno != ENOENT) ? rc : 0; > > } > > > > @@ -1488,7 +1488,7 @@ rebuild: > > retval = semanage_copy_file(path, > > semanage_path(SEMANAGE_TMP, > > SEMANAGE_STORE_SEUSERS), > > - 0); > > + 0, false); > > if (retval < 0) > > goto cleanup; > > pseusers->dtable->drop_cache(pseusers->dbase); > > @@ -1506,7 +1506,7 @@ rebuild: > > retval = semanage_copy_file(path, > > semanage_path(SEMANAGE_TMP, > > SEMANAGE_USERS_EXTRA), > > - 0); > > + 0, false); > > if (retval < 0) > > goto cleanup; > > pusers_extra->dtable->drop_cache(pusers_extra->dbase); > > @@ -1595,7 +1595,7 @@ rebuild: > > > > retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL), > > semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_KERNEL), > > - sh->conf->file_mode); > > + sh->conf->file_mode, false); > > if (retval < 0) { > > goto cleanup; > > } > > @@ -1634,7 +1634,7 @@ rebuild: > > retval = semanage_copy_file( > > semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_HOMEDIRS), > > semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_HOMEDIRS), > > - sh->conf->file_mode); > > + sh->conf->file_mode, false); > > if (retval < 0) { > > goto cleanup; > > } > > diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c > > index 859c0a22..cd5e46bb 100644 > > --- a/libsemanage/src/semanage_store.c > > +++ b/libsemanage/src/semanage_store.c > > @@ -707,7 +707,8 @@ static int semanage_filename_select(const struct dirent *d) > > > > /* Copies a file from src to dst. If dst already exists then > > * overwrite it. Returns 0 on success, -1 on error. */ > > -int semanage_copy_file(const char *src, const char *dst, mode_t mode) > > +int semanage_copy_file(const char *src, const char *dst, mode_t mode, > > + bool syncrequired) > > { > > int in, out, retval = 0, amount_read, n, errsv = errno; > > char tmp[PATH_MAX]; > > @@ -735,8 +736,11 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode) > > } > > umask(mask); > > while (retval == 0 && (amount_read = read(in, buf, sizeof(buf))) > 0) { > > - if (write(out, buf, amount_read) < 0) { > > - errsv = errno; > > + if (write(out, buf, amount_read) != amount_read) { > > + if (errno) > > + errsv = errno; > > + else > > + errsv = EIO; > > retval = -1; > > } > > } > > @@ -745,6 +749,10 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode) > > retval = -1; > > } > > close(in); > > + if (syncrequired && fsync(out) < 0) { > > + errsv = errno; > > + retval = -1; > > + } > > The patch looks good to me, even though I am wondering whether it > makes sense to call fsync() if an error happens (by changing the > condition to "if (!retval && syncrequired && fsync(out) < 0)"). > Anyway, this is a minor comment and I am fine with the other changes. > > Acked-by: Nicolas Iooss <nicolas.iooss@xxxxxxx> > Applied. > > if (close(out) < 0) { > > errsv = errno; > > retval = -1; > > @@ -811,7 +819,8 @@ static int semanage_copy_dir_flags(const char *src, const char *dst, int flag) > > umask(mask); > > } else if (S_ISREG(sb.st_mode) && flag == 1) { > > mask = umask(0077); > > - if (semanage_copy_file(path, path2, sb.st_mode) < 0) { > > + if (semanage_copy_file(path, path2, sb.st_mode, > > + false) < 0) { > > umask(mask); > > goto cleanup; > > } > > @@ -1639,7 +1648,8 @@ static int semanage_install_final_tmp(semanage_handle_t * sh) > > goto cleanup; > > } > > > > - ret = semanage_copy_file(src, dst, sh->conf->file_mode); > > + ret = semanage_copy_file(src, dst, sh->conf->file_mode, > > + true); > > if (ret < 0) { > > ERR(sh, "Could not copy %s to %s.", src, dst); > > goto cleanup; > > diff --git a/libsemanage/src/semanage_store.h b/libsemanage/src/semanage_store.h > > index 34bf8523..b9ec5664 100644 > > --- a/libsemanage/src/semanage_store.h > > +++ b/libsemanage/src/semanage_store.h > > @@ -24,6 +24,7 @@ > > #ifndef SEMANAGE_MODULE_STORE_H > > #define SEMANAGE_MODULE_STORE_H > > > > +#include <stdbool.h> > > #include <sys/time.h> > > #include <sepol/module.h> > > #include <sepol/cil/cil.h> > > @@ -162,6 +163,7 @@ int semanage_nc_sort(semanage_handle_t * sh, > > size_t buf_len, > > char **sorted_buf, size_t * sorted_buf_len); > > > > -int semanage_copy_file(const char *src, const char *dst, mode_t mode); > > +int semanage_copy_file(const char *src, const char *dst, mode_t mode, > > + bool syncrequired); > > > > #endif > > -- > > 2.23.3 > > > -- () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments
Attachment:
signature.asc
Description: PGP signature
