| Message ID | 20220916141308.72606-1-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] fixfiles: Unmount temporary bind mounts on SIGINT | expand |
On Fri, 16 Sept 2022 at 16:14, Petr Lautrbach <plautrba@redhat.com> wrote: > > `fixfiles -M relabel` temporary bind mounts filestems before relabeling > but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits > CTRL-C. It means that if the user run `fixfiles -M relabel` again and > answered Y to clean out /tmp directory, it would remove all data from > mounted fs. > > This patch changes the location where `fixfiles` mounts fs to /run and > adds a handler for exit signals which tries to umount fs mounted by > `fixfiles`. What about additionally using mount namespaces, if available: @@ -256,10 +256,16 @@ test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 mkdir -p "${TMP_MOUNT}${m}" || exit 1 - mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1 - ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}" - umount "${TMP_MOUNT}${m}" || exit 1 - rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." + if ! unshare --mount /bin/sh -c "mount --bind \"${m}\" \"${TMP_MOUNT}${m}\" || exit 1 && ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r \"${TMP_MOUNT}\" \"${TMP_MOUNT}${m}\" || true"; then + echo "Creating new mount namespace failed, operating in root namespace" + mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1 + ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}" + umount "${TMP_MOUNT}${m}" || exit 1 + fi + if [ "${m}" != '/' ]; then + rmdir "${TMP_MOUNT}${m}" || echo "Error cleaning up ${TMP_MOUNT}${m}." + fi + rmdir "${TMP_MOUNT}" || echo "Error cleaning up ${TMP_MOUNT}." done; fi else ? > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > --- > v2: > > - set trap on EXIT instead of SIGINT > > v3: > > - use /run instead of /tmp for mountpoints > > > policycoreutils/scripts/fixfiles | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > index c72ca0eb9d61..acf5f0996c19 100755 > --- a/policycoreutils/scripts/fixfiles > +++ b/policycoreutils/scripts/fixfiles > @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' > [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr > } > > +# unmount tmp bind mount before exit > +umount_TMP_MOUNT() { > + if [ -n "$TMP_MOUNT" ]; then > + umount "${TMP_MOUNT}${m}" || exit 130 > + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > + fi > + exit 130 > +} > + > # > # restore > # if called with -n will only check file context > @@ -251,8 +260,9 @@ case "$RESTORE_MODE" in > else > # we bind mount so we can fix the labels of files that have already been > # mounted over > + trap umount_TMP_MOUNT EXIT > for m in `echo $FILESYSTEMSRW`; do > - TMP_MOUNT="$(mktemp -d)" > + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" > test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > > mkdir -p "${TMP_MOUNT}${m}" || exit 1 > @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in > umount "${TMP_MOUNT}${m}" || exit 1 > rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > done; > + trap EXIT > fi > else > echo >&2 "fixfiles: No suitable file systems found" > -- > 2.37.3 >
(Resending without HTML...) On Fri, Sep 16, 2022 at 5:37 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Fri, 16 Sept 2022 at 16:14, Petr Lautrbach <plautrba@redhat.com> wrote: > > > > `fixfiles -M relabel` temporary bind mounts filestems before relabeling > > but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits > > CTRL-C. It means that if the user run `fixfiles -M relabel` again and > > answered Y to clean out /tmp directory, it would remove all data from > > mounted fs. > > > > This patch changes the location where `fixfiles` mounts fs to /run and > > adds a handler for exit signals which tries to umount fs mounted by > > `fixfiles`. > > What about additionally using mount namespaces, if available: What benefit would it bring? (Sorry, I'm not very familiar with mount namespaces.) > > @@ -256,10 +256,16 @@ > test -z ${TMP_MOUNT+x} && echo "Unable to find > temporary directory!" && exit 1 > > mkdir -p "${TMP_MOUNT}${m}" || exit 1 > - mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1 > - ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} > ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}" > - umount "${TMP_MOUNT}${m}" || exit 1 > - rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > + if ! unshare --mount /bin/sh -c "mount --bind > \"${m}\" \"${TMP_MOUNT}${m}\" || exit 1 && ${SETFILES} ${VERBOSE} > ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r \"${TMP_MOUNT}\" > \"${TMP_MOUNT}${m}\" || true"; then > + echo "Creating new mount namespace failed, > operating in root namespace" > + mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1 > + ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} > ${FORCEFLAG} ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" > "${TMP_MOUNT}${m}" > + umount "${TMP_MOUNT}${m}" || exit 1 > + fi > + if [ "${m}" != '/' ]; then > + rmdir "${TMP_MOUNT}${m}" || echo "Error > cleaning up ${TMP_MOUNT}${m}." > + fi > + rmdir "${TMP_MOUNT}" || echo "Error cleaning > up ${TMP_MOUNT}." > done; > fi > else > > ? > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > > > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > > --- > > v2: > > > > - set trap on EXIT instead of SIGINT > > > > v3: > > > > - use /run instead of /tmp for mountpoints > > > > > > policycoreutils/scripts/fixfiles | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > > index c72ca0eb9d61..acf5f0996c19 100755 > > --- a/policycoreutils/scripts/fixfiles > > +++ b/policycoreutils/scripts/fixfiles > > @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' > > [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr > > } > > > > +# unmount tmp bind mount before exit > > +umount_TMP_MOUNT() { > > + if [ -n "$TMP_MOUNT" ]; then > > + umount "${TMP_MOUNT}${m}" || exit 130 > > + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > > + fi > > + exit 130 > > +} > > + > > # > > # restore > > # if called with -n will only check file context > > @@ -251,8 +260,9 @@ case "$RESTORE_MODE" in > > else > > # we bind mount so we can fix the labels of files that have already been > > # mounted over > > + trap umount_TMP_MOUNT EXIT > > for m in `echo $FILESYSTEMSRW`; do > > - TMP_MOUNT="$(mktemp -d)" > > + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" > > test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > > > > mkdir -p "${TMP_MOUNT}${m}" || exit 1 > > @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in > > umount "${TMP_MOUNT}${m}" || exit 1 > > rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > > done; > > + trap EXIT > > fi > > else > > echo >&2 "fixfiles: No suitable file systems found" > > -- > > 2.37.3 > > > -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Mon, 19 Sept 2022 at 10:39, Ondrej Mosnacek <omosnace@redhat.com> wrote: > > (Resending without HTML...) > > On Fri, Sep 16, 2022 at 5:37 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > On Fri, 16 Sept 2022 at 16:14, Petr Lautrbach <plautrba@redhat.com> wrote: > > > > > > `fixfiles -M relabel` temporary bind mounts filestems before relabeling > > > but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits > > > CTRL-C. It means that if the user run `fixfiles -M relabel` again and > > > answered Y to clean out /tmp directory, it would remove all data from > > > mounted fs. > > > > > > This patch changes the location where `fixfiles` mounts fs to /run and > > > adds a handler for exit signals which tries to umount fs mounted by > > > `fixfiles`. > > > > What about additionally using mount namespaces, if available: > > What benefit would it bring? (Sorry, I'm not very familiar with mount > namespaces.) The bind mounts will only be visible to the process and its children used in the unshare command (and processes explicitly joining that mount namespace) and not to other (parallel or subsequent) instances of fixfiles. > > > > > @@ -256,10 +256,16 @@ > > test -z ${TMP_MOUNT+x} && echo "Unable to find > > temporary directory!" && exit 1 > > > > mkdir -p "${TMP_MOUNT}${m}" || exit 1 > > - mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1 > > - ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} > > ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}" > > - umount "${TMP_MOUNT}${m}" || exit 1 > > - rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > > + if ! unshare --mount /bin/sh -c "mount --bind > > \"${m}\" \"${TMP_MOUNT}${m}\" || exit 1 && ${SETFILES} ${VERBOSE} > > ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r \"${TMP_MOUNT}\" > > \"${TMP_MOUNT}${m}\" || true"; then > > + echo "Creating new mount namespace failed, > > operating in root namespace" > > + mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1 > > + ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} > > ${FORCEFLAG} ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" > > "${TMP_MOUNT}${m}" > > + umount "${TMP_MOUNT}${m}" || exit 1 > > + fi > > + if [ "${m}" != '/' ]; then > > + rmdir "${TMP_MOUNT}${m}" || echo "Error > > cleaning up ${TMP_MOUNT}${m}." > > + fi > > + rmdir "${TMP_MOUNT}" || echo "Error cleaning > > up ${TMP_MOUNT}." > > done; > > fi > > else > > > > ? > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > > > > > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > > > --- > > > v2: > > > > > > - set trap on EXIT instead of SIGINT > > > > > > v3: > > > > > > - use /run instead of /tmp for mountpoints > > > > > > > > > policycoreutils/scripts/fixfiles | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > > > index c72ca0eb9d61..acf5f0996c19 100755 > > > --- a/policycoreutils/scripts/fixfiles > > > +++ b/policycoreutils/scripts/fixfiles > > > @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' > > > [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr > > > } > > > > > > +# unmount tmp bind mount before exit > > > +umount_TMP_MOUNT() { > > > + if [ -n "$TMP_MOUNT" ]; then > > > + umount "${TMP_MOUNT}${m}" || exit 130 > > > + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > > > + fi > > > + exit 130 > > > +} > > > + > > > # > > > # restore > > > # if called with -n will only check file context > > > @@ -251,8 +260,9 @@ case "$RESTORE_MODE" in > > > else > > > # we bind mount so we can fix the labels of files that have already been > > > # mounted over > > > + trap umount_TMP_MOUNT EXIT > > > for m in `echo $FILESYSTEMSRW`; do > > > - TMP_MOUNT="$(mktemp -d)" > > > + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" > > > test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > > > > > > mkdir -p "${TMP_MOUNT}${m}" || exit 1 > > > @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in > > > umount "${TMP_MOUNT}${m}" || exit 1 > > > rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." > > > done; > > > + trap EXIT > > > fi > > > else > > > echo >&2 "fixfiles: No suitable file systems found" > > > -- > > > 2.37.3 > > > > > > > -- > Ondrej Mosnacek > Senior Software Engineer, Linux Security - SELinux kernel > Red Hat, Inc. >
diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles index c72ca0eb9d61..acf5f0996c19 100755 --- a/policycoreutils/scripts/fixfiles +++ b/policycoreutils/scripts/fixfiles @@ -207,6 +207,15 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' ' [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr } +# unmount tmp bind mount before exit +umount_TMP_MOUNT() { + if [ -n "$TMP_MOUNT" ]; then + umount "${TMP_MOUNT}${m}" || exit 130 + rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." + fi + exit 130 +} + # # restore # if called with -n will only check file context @@ -251,8 +260,9 @@ case "$RESTORE_MODE" in else # we bind mount so we can fix the labels of files that have already been # mounted over + trap umount_TMP_MOUNT EXIT for m in `echo $FILESYSTEMSRW`; do - TMP_MOUNT="$(mktemp -d)" + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 mkdir -p "${TMP_MOUNT}${m}" || exit 1 @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in umount "${TMP_MOUNT}${m}" || exit 1 rm -rf "${TMP_MOUNT}" || echo "Error cleaning up." done; + trap EXIT fi else echo >&2 "fixfiles: No suitable file systems found"
`fixfiles -M relabel` temporary bind mounts filestems before relabeling but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits CTRL-C. It means that if the user run `fixfiles -M relabel` again and answered Y to clean out /tmp directory, it would remove all data from mounted fs. This patch changes the location where `fixfiles` mounts fs to /run and adds a handler for exit signals which tries to umount fs mounted by `fixfiles`. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 Signed-off-by: Petr Lautrbach <plautrba@redhat.com> --- v2: - set trap on EXIT instead of SIGINT v3: - use /run instead of /tmp for mountpoints policycoreutils/scripts/fixfiles | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)