* [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT
@ 2022-11-07 9:25 Petr Lautrbach
2022-11-07 11:10 ` Petr Lautrbach
2022-11-21 13:50 ` James Carter
0 siblings, 2 replies; 7+ messages in thread
From: Petr Lautrbach @ 2022-11-07 9:25 UTC (permalink / raw)
To: selinux; +Cc: Petr Lautrbach
`fixfiles -M relabel` temporary bind mounts file systems before
relabeling, but it left the / directory mounted in /tmp/tmp.XXXX when a
user hit 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, uses
private mount namespace via unshare 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: fixed issues reported by Christian Göttsche <cgzones@googlemail.com>
policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
index c72ca0eb9d61..166af6f360a2 100755
--- a/policycoreutils/scripts/fixfiles
+++ b/policycoreutils/scripts/fixfiles
@@ -207,6 +207,25 @@ 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
+}
+
+fix_labels_on_mountpoint() {
+ 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."
+}
+export -f fix_labels_on_mountpoint
+
#
# restore
# if called with -n will only check file context
@@ -252,14 +271,15 @@ case "$RESTORE_MODE" in
# we bind mount so we can fix the labels of files that have already been
# mounted over
for m in `echo $FILESYSTEMSRW`; do
- TMP_MOUNT="$(mktemp -d)"
- 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."
+ TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)"
+ export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m
+ if type unshare &> /dev/null; then
+ unshare -m bash -c "fix_labels_on_mountpoint $*" || exit $?
+ else
+ trap umount_TMP_MOUNT EXIT
+ fix_labels_on_mountpoint $*
+ trap EXIT
+ fi
done;
fi
else
--
2.37.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT
2022-11-07 9:25 [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT Petr Lautrbach
@ 2022-11-07 11:10 ` Petr Lautrbach
2022-11-18 10:02 ` Petr Lautrbach
2022-11-21 13:50 ` James Carter
1 sibling, 1 reply; 7+ messages in thread
From: Petr Lautrbach @ 2022-11-07 11:10 UTC (permalink / raw)
To: selinux
Petr Lautrbach <plautrba@redhat.com> writes:
> `fixfiles -M relabel` temporary bind mounts file systems before
> relabeling, but it left the / directory mounted in /tmp/tmp.XXXX when a
> user hit 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, uses
> private mount namespace via unshare 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>
> ---
Actually, it's v5:
v2:
- set trap on EXIT instead of SIGINT
v3:
- use /run instead of /tmp for mountpoints
v4:
- use mount namespace as suggested by Christian Göttsche <cgzones@googlemail.com>
v5
- fixed issues reported by Christian Göttsche <cgzones@googlemail.com>
>
>
> policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
> index c72ca0eb9d61..166af6f360a2 100755
> --- a/policycoreutils/scripts/fixfiles
> +++ b/policycoreutils/scripts/fixfiles
> @@ -207,6 +207,25 @@ 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
> +}
> +
> +fix_labels_on_mountpoint() {
> + 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."
> +}
> +export -f fix_labels_on_mountpoint
> +
> #
> # restore
> # if called with -n will only check file context
> @@ -252,14 +271,15 @@ case "$RESTORE_MODE" in
> # we bind mount so we can fix the labels of files that have already been
> # mounted over
> for m in `echo $FILESYSTEMSRW`; do
> - TMP_MOUNT="$(mktemp -d)"
> - 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."
> + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)"
> + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m
> + if type unshare &> /dev/null; then
> + unshare -m bash -c "fix_labels_on_mountpoint $*" || exit $?
> + else
> + trap umount_TMP_MOUNT EXIT
> + fix_labels_on_mountpoint $*
> + trap EXIT
> + fi
> done;
> fi
> else
> --
> 2.37.3
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT
2022-11-07 11:10 ` Petr Lautrbach
@ 2022-11-18 10:02 ` Petr Lautrbach
2022-11-18 17:01 ` Christian Göttsche
0 siblings, 1 reply; 7+ messages in thread
From: Petr Lautrbach @ 2022-11-18 10:02 UTC (permalink / raw)
To: selinux
Petr Lautrbach <plautrba@redhat.com> writes:
> Petr Lautrbach <plautrba@redhat.com> writes:
>
>> `fixfiles -M relabel` temporary bind mounts file systems before
>> relabeling, but it left the / directory mounted in /tmp/tmp.XXXX when a
>> user hit 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, uses
>> private mount namespace via unshare 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>
>> ---
>
> Actually, it's v5:
>
> v2:
>
> - set trap on EXIT instead of SIGINT
>
> v3:
>
> - use /run instead of /tmp for mountpoints
>
> v4:
>
> - use mount namespace as suggested by Christian Göttsche <cgzones@googlemail.com>
>
> v5
>
> - fixed issues reported by Christian Göttsche <cgzones@googlemail.com>
>
Any objections?
>>
>>
>> policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++-------
>> 1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
>> index c72ca0eb9d61..166af6f360a2 100755
>> --- a/policycoreutils/scripts/fixfiles
>> +++ b/policycoreutils/scripts/fixfiles
>> @@ -207,6 +207,25 @@ 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
>> +}
>> +
>> +fix_labels_on_mountpoint() {
>> + 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."
>> +}
>> +export -f fix_labels_on_mountpoint
>> +
>> #
>> # restore
>> # if called with -n will only check file context
>> @@ -252,14 +271,15 @@ case "$RESTORE_MODE" in
>> # we bind mount so we can fix the labels of files that have already been
>> # mounted over
>> for m in `echo $FILESYSTEMSRW`; do
>> - TMP_MOUNT="$(mktemp -d)"
>> - 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."
>> + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)"
>> + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m
>> + if type unshare &> /dev/null; then
>> + unshare -m bash -c "fix_labels_on_mountpoint $*" || exit $?
>> + else
>> + trap umount_TMP_MOUNT EXIT
>> + fix_labels_on_mountpoint $*
>> + trap EXIT
>> + fi
>> done;
>> fi
>> else
>> --
>> 2.37.3
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT
2022-11-18 10:02 ` Petr Lautrbach
@ 2022-11-18 17:01 ` Christian Göttsche
0 siblings, 0 replies; 7+ messages in thread
From: Christian Göttsche @ 2022-11-18 17:01 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: selinux
On Fri, 18 Nov 2022 at 11:03, Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Petr Lautrbach <plautrba@redhat.com> writes:
>
> > Petr Lautrbach <plautrba@redhat.com> writes:
> >
> >> `fixfiles -M relabel` temporary bind mounts file systems before
> >> relabeling, but it left the / directory mounted in /tmp/tmp.XXXX when a
> >> user hit 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, uses
> >> private mount namespace via unshare 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>
> >> ---
> >
> > Actually, it's v5:
> >
> > v2:
> >
> > - set trap on EXIT instead of SIGINT
> >
> > v3:
> >
> > - use /run instead of /tmp for mountpoints
> >
> > v4:
> >
> > - use mount namespace as suggested by Christian Göttsche <cgzones@googlemail.com>
> >
> > v5
> >
> > - fixed issues reported by Christian Göttsche <cgzones@googlemail.com>
> >
>
> Any objections?
Works fine for me.
Tested-by: Christian Göttsche <cgzones@googlemail.com>
>
>
> >>
> >>
> >> policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++-------
> >> 1 file changed, 28 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
> >> index c72ca0eb9d61..166af6f360a2 100755
> >> --- a/policycoreutils/scripts/fixfiles
> >> +++ b/policycoreutils/scripts/fixfiles
> >> @@ -207,6 +207,25 @@ 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
> >> +}
> >> +
> >> +fix_labels_on_mountpoint() {
> >> + 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."
> >> +}
> >> +export -f fix_labels_on_mountpoint
> >> +
> >> #
> >> # restore
> >> # if called with -n will only check file context
> >> @@ -252,14 +271,15 @@ case "$RESTORE_MODE" in
> >> # we bind mount so we can fix the labels of files that have already been
> >> # mounted over
> >> for m in `echo $FILESYSTEMSRW`; do
> >> - TMP_MOUNT="$(mktemp -d)"
> >> - 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."
> >> + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)"
> >> + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m
> >> + if type unshare &> /dev/null; then
> >> + unshare -m bash -c "fix_labels_on_mountpoint $*" || exit $?
> >> + else
> >> + trap umount_TMP_MOUNT EXIT
> >> + fix_labels_on_mountpoint $*
> >> + trap EXIT
> >> + fi
> >> done;
> >> fi
> >> else
> >> --
> >> 2.37.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT
2022-11-07 9:25 [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT Petr Lautrbach
2022-11-07 11:10 ` Petr Lautrbach
@ 2022-11-21 13:50 ` James Carter
2022-11-23 15:05 ` James Carter
1 sibling, 1 reply; 7+ messages in thread
From: James Carter @ 2022-11-21 13:50 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: selinux
On Mon, Nov 7, 2022 at 4:31 AM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> `fixfiles -M relabel` temporary bind mounts file systems before
> relabeling, but it left the / directory mounted in /tmp/tmp.XXXX when a
> user hit 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, uses
> private mount namespace via unshare 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>
Acked-by: James Carter <jwcart2@gmail.com>
> ---
>
> v2: fixed issues reported by Christian Göttsche <cgzones@googlemail.com>
>
>
> policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
> index c72ca0eb9d61..166af6f360a2 100755
> --- a/policycoreutils/scripts/fixfiles
> +++ b/policycoreutils/scripts/fixfiles
> @@ -207,6 +207,25 @@ 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
> +}
> +
> +fix_labels_on_mountpoint() {
> + 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."
> +}
> +export -f fix_labels_on_mountpoint
> +
> #
> # restore
> # if called with -n will only check file context
> @@ -252,14 +271,15 @@ case "$RESTORE_MODE" in
> # we bind mount so we can fix the labels of files that have already been
> # mounted over
> for m in `echo $FILESYSTEMSRW`; do
> - TMP_MOUNT="$(mktemp -d)"
> - 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."
> + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)"
> + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m
> + if type unshare &> /dev/null; then
> + unshare -m bash -c "fix_labels_on_mountpoint $*" || exit $?
> + else
> + trap umount_TMP_MOUNT EXIT
> + fix_labels_on_mountpoint $*
> + trap EXIT
> + fi
> done;
> fi
> else
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT
2022-11-21 13:50 ` James Carter
@ 2022-11-23 15:05 ` James Carter
0 siblings, 0 replies; 7+ messages in thread
From: James Carter @ 2022-11-23 15:05 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: selinux
On Mon, Nov 21, 2022 at 8:50 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Mon, Nov 7, 2022 at 4:31 AM Petr Lautrbach <plautrba@redhat.com> wrote:
> >
> > `fixfiles -M relabel` temporary bind mounts file systems before
> > relabeling, but it left the / directory mounted in /tmp/tmp.XXXX when a
> > user hit 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, uses
> > private mount namespace via unshare 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>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
Merged.
Thanks,
Jim
> > ---
> >
> > v2: fixed issues reported by Christian Göttsche <cgzones@googlemail.com>
> >
> >
> > policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++-------
> > 1 file changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
> > index c72ca0eb9d61..166af6f360a2 100755
> > --- a/policycoreutils/scripts/fixfiles
> > +++ b/policycoreutils/scripts/fixfiles
> > @@ -207,6 +207,25 @@ 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
> > +}
> > +
> > +fix_labels_on_mountpoint() {
> > + 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."
> > +}
> > +export -f fix_labels_on_mountpoint
> > +
> > #
> > # restore
> > # if called with -n will only check file context
> > @@ -252,14 +271,15 @@ case "$RESTORE_MODE" in
> > # we bind mount so we can fix the labels of files that have already been
> > # mounted over
> > for m in `echo $FILESYSTEMSRW`; do
> > - TMP_MOUNT="$(mktemp -d)"
> > - 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."
> > + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)"
> > + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m
> > + if type unshare &> /dev/null; then
> > + unshare -m bash -c "fix_labels_on_mountpoint $*" || exit $?
> > + else
> > + trap umount_TMP_MOUNT EXIT
> > + fix_labels_on_mountpoint $*
> > + trap EXIT
> > + fi
> > done;
> > fi
> > else
> > --
> > 2.37.3
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT
@ 2022-09-15 13:18 Ondrej Mosnacek
2022-09-15 16:37 ` [PATCH v2] " Petr Lautrbach
0 siblings, 1 reply; 7+ messages in thread
From: Ondrej Mosnacek @ 2022-09-15 13:18 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list
On Thu, Sep 15, 2022 at 2:45 PM 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.
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
> policycoreutils/scripts/fixfiles | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
> index c72ca0eb9d61..6811921970f2 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,6 +260,7 @@ 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 SIGINT
> for m in `echo $FILESYSTEMSRW`; do
> TMP_MOUNT="$(mktemp -d)"
> test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && 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 SIGINT
> fi
> else
> echo >&2 "fixfiles: No suitable file systems found"
> --
> 2.37.3
>
And what if the fixfiles process is terminated via SIGTERM or even
SIGKILL? Or a power failure occurs at the wrong time? What if some
other process leaves behind a bind mount / other mount in /tmp?
My suggestion would be to:
1) Change the trap from SIGINT to EXIT. That will cover both SIGTERM and SIGINT.
2) Additionally modify fullrelabel() to not traverse across mounts
when doing the removing (+ possibly exit with an error whenever a
mountpoint is encountered - OR - try to unmount the mounts instead of
removing their contents).
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT
2022-09-15 13:18 [PATCH] " Ondrej Mosnacek
@ 2022-09-15 16:37 ` Petr Lautrbach
0 siblings, 0 replies; 7+ messages in thread
From: Petr Lautrbach @ 2022-09-15 16:37 UTC (permalink / raw)
To: selinux; +Cc: Petr Lautrbach
`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.
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
policycoreutils/scripts/fixfiles | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
index c72ca0eb9d61..d763fa83eefd 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,6 +260,7 @@ 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)"
test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-23 15:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-07 9:25 [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT Petr Lautrbach
2022-11-07 11:10 ` Petr Lautrbach
2022-11-18 10:02 ` Petr Lautrbach
2022-11-18 17:01 ` Christian Göttsche
2022-11-21 13:50 ` James Carter
2022-11-23 15:05 ` James Carter
-- strict thread matches above, loose matches on Subject: below --
2022-09-15 13:18 [PATCH] " Ondrej Mosnacek
2022-09-15 16:37 ` [PATCH v2] " 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.