On Wed, May 13, 2020 at 2:52 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > On Wed, May 13, 2020 at 8:46 PM Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > > > > From: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > --- > > 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.
