* [PATCH v2] libsemanage: fsync final files before rename
@ 2020-05-13 19:34 Smalley
2020-05-14 18:44 ` Nicolas Iooss
0 siblings, 1 reply; 3+ messages in thread
From: Smalley @ 2020-05-13 19:34 UTC (permalink / raw)
To: selinux; +Cc: Stephen Smalley
From: Stephen Smalley <stephen.smalley.work@gmail.com>
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@gmail.com>
---
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;
+ }
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] libsemanage: fsync final files before rename
2020-05-13 19:34 [PATCH v2] libsemanage: fsync final files before rename Smalley
@ 2020-05-14 18:44 ` Nicolas Iooss
2020-05-15 13:42 ` Petr Lautrbach
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Iooss @ 2020-05-14 18:44 UTC (permalink / raw)
To: Smalley; +Cc: SElinux list
On Wed, May 13, 2020 at 9:34 PM Smalley <stephen.smalley.work@gmail.com> wrote:
>
> From: Stephen Smalley <stephen.smalley.work@gmail.com>
>
> 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@gmail.com>
> ---
> 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@m4x.org>
Thanks!
Nicolas
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] libsemanage: fsync final files before rename
2020-05-14 18:44 ` Nicolas Iooss
@ 2020-05-15 13:42 ` Petr Lautrbach
0 siblings, 0 replies; 3+ messages in thread
From: Petr Lautrbach @ 2020-05-15 13:42 UTC (permalink / raw)
To: SElinux list; +Cc: Smalley, Nicolas Iooss
[-- Attachment #1: Type: text/plain, Size: 8304 bytes --]
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@gmail.com> wrote:
> >
> > From: Stephen Smalley <stephen.smalley.work@gmail.com>
> >
> > 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@gmail.com>
> > ---
> > 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@m4x.org>
>
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-15 13:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 19:34 [PATCH v2] libsemanage: fsync final files before rename Smalley
2020-05-14 18:44 ` Nicolas Iooss
2020-05-15 13:42 ` Petr Lautrbach
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.