All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] libsemanage: fsync before rename
@ 2020-05-13 18:46 Smalley
  2020-05-13 18:52 ` Nicolas Iooss
  0 siblings, 1 reply; 3+ messages in thread
From: Smalley @ 2020-05-13 18:46 UTC (permalink / raw)
  To: selinux; +Cc: Stephen Smalley

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;
 		}
@@ -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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] libsemanage: fsync before rename
  2020-05-13 18:46 [PATCH] libsemanage: fsync before rename Smalley
@ 2020-05-13 18:52 ` Nicolas Iooss
  2020-05-13 19:08   ` Stephen Smalley
  0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Iooss @ 2020-05-13 18:52 UTC (permalink / raw)
  To: Smalley; +Cc: SElinux list

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
>


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] libsemanage: fsync before rename
  2020-05-13 18:52 ` Nicolas Iooss
@ 2020-05-13 19:08   ` Stephen Smalley
  0 siblings, 0 replies; 3+ messages in thread
From: Stephen Smalley @ 2020-05-13 19:08 UTC (permalink / raw)
  To: Nicolas Iooss; +Cc: SElinux list

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-05-13 19:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 18:46 [PATCH] libsemanage: fsync before rename Smalley
2020-05-13 18:52 ` Nicolas Iooss
2020-05-13 19:08   ` Stephen Smalley

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.