| Message ID | 5ebc4079.1c69fb81.c8782.38eb@mx.google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libsemanage: fsync before rename | expand |
On Wed, May 13, 2020 at 8:46 PM Smalley <stephen.smalley.work@gmail.com> wrote: > > From: Stephen Smalley <stephen.smalley.work@gmail.com> > > Prior to rename(2)'ing new 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. 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> > --- > libsemanage/src/semanage_store.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c > index 859c0a22..3cac36d4 100644 > --- a/libsemanage/src/semanage_store.c > +++ b/libsemanage/src/semanage_store.c > @@ -735,7 +735,7 @@ 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) { > + if (write(out, buf, amount_read) != amount_read) { > errsv = errno; > retval = -1; > } If I remember correctly, errno is not defined if a short write happens. If this is confirmed and if you want to keep the patch short, you could for example use errsv = EIO if write() returned a value different from -1 and from amount_read. Thanks, Nicolas > @@ -745,6 +745,10 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode) > retval = -1; > } > close(in); > + if (fsync(out) < 0) { > + errsv = errno; > + retval = -1; > + } > if (close(out) < 0) { > errsv = errno; > retval = -1; > -- > 2.23.3 >
On Wed, May 13, 2020 at 2:52 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Wed, May 13, 2020 at 8:46 PM Smalley <stephen.smalley.work@gmail.com> wrote: > > > > From: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > Prior to rename(2)'ing new 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. 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> > > --- > > libsemanage/src/semanage_store.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c > > index 859c0a22..3cac36d4 100644 > > --- a/libsemanage/src/semanage_store.c > > +++ b/libsemanage/src/semanage_store.c > > @@ -735,7 +735,7 @@ 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) { > > + if (write(out, buf, amount_read) != amount_read) { > > errsv = errno; > > retval = -1; > > } > > If I remember correctly, errno is not defined if a short write > happens. If this is confirmed and if you want to keep the patch short, > you could for example use errsv = EIO if write() returned a value > different from -1 and from amount_read. True. It also occurred to me that this is too heavyweight given how widely semanage_copy_file() is used within libsemanage; performing a fsync(2) for every one of these file copies will be very expensive. We only really care about it when copying the final files to /etc/selinux, not for the copying under /var/lib/selinux IIUC. So I guess I need a bool argument or similar to semanage_copy_file() to indicate when a fsync is required. Even with this change, installing the final SELinux policy files under /etc/selinux won't be fully atomic; one can still end up with a mix of old and new (e.g. new policy.32 with old file_contexts). But that's out of scope for this particular bug.
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c index 859c0a22..3cac36d4 100644 --- a/libsemanage/src/semanage_store.c +++ b/libsemanage/src/semanage_store.c @@ -735,7 +735,7 @@ 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) { + if (write(out, buf, amount_read) != amount_read) { errsv = errno; retval = -1; } @@ -745,6 +745,10 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode) retval = -1; } close(in); + if (fsync(out) < 0) { + errsv = errno; + retval = -1; + } if (close(out) < 0) { errsv = errno; retval = -1;