[PATCH] kdumpctl: Avoid leaking fd to subshell

[Vivek Goyal]

On Tue, Nov 26, 2013 at 12:23:11AM +0800, WANG Chao wrote:
> We only allow one instance of kdump service running at each time by
> flock /var/lock/kdump which is opened as fd 9 in kdumpctl script.
> 
> However a leaking fd issue has been discovered by SELinux:
> 
> When executing a specific shell command (not the shell built-in but
> provided by other packages, in this case - restorecon) in kdumpctl,
> current shell will fork a new subshell for executing and
> the subshell will inherit open fd 9 from parent shell. And SELinux
> detects that subshell is holding the open fd and consider fd 9 is
> leaked.
> 
> To avoid this kind of leaking, the most easy way seems to be breaking our
> kdumpctl code out into two parts:
> - A top level parent shell, which is only used to deal with the lock and
>   invoking the subshell below.
> - A 2nd tier level subshell, which is closing the inherited open fd at
>   very first and doing the rest of the kdumpctl job. So that it isn't
>   leaking fd to its subshell when executing like restorecon, etc.
> 
> To be easy to understand, the callgraph is roughly like below:
> [..]
> --> open(9)
> --> flock(9)
> --> fork
>   --> close(9)      <-- we close 9 right here
>   --> main()        <-- we're now doing the real job
>   --> [..]
>   --> fork()
>     --> restorecon  <-- we don't leak fd 9 to child process
>   --> [..]
> --> [..]
> 
> As shown above, a wrapper main() is added as the 2nd tier level shell in
> this kind of call model. So we can completely avoid leaking fd to
> subshell.
> 
> Signed-off-by: WANG Chao <chaowang at redhat.com>

Hi Chao,

This looks good to me. 

Acked-by: Vivek Goyal <vgoyal at redhat.com>

Thanks
Vivek

> ---
>  kdumpctl | 91 ++++++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 49 insertions(+), 42 deletions(-)
> 
> diff --git a/kdumpctl b/kdumpctl
> index 358ef05..46ae633 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -543,52 +543,59 @@ if [ ! -f "$KDUMP_CONFIG_FILE" ]; then
>  	exit 1
>  fi
>  
> -# Other kdumpctl instances will block in queue, until this one exits
> -single_instance_lock
> -
> -case "$1" in
> -  start)
> -	if [ -s /proc/vmcore ]; then
> -		save_core
> -		reboot
> -	else
> -		start
> -	fi
> -	;;
> -  stop)
> -	stop
> -	;;
> -  status)
> -	EXIT_CODE=0
> -	status
> -	case "$?" in
> -	0)
> -		echo "Kdump is operational"
> +main ()
> +{
> +	case "$1" in
> +	  start)
> +		if [ -s /proc/vmcore ]; then
> +			save_core
> +			reboot
> +		else
> +			start
> +		fi
> +		;;
> +	  stop)
> +		stop
> +		;;
> +	  status)
>  		EXIT_CODE=0
> +		status
> +		case "$?" in
> +		  0)
> +			echo "Kdump is operational"
> +			EXIT_CODE=0
> +			;;
> +		  1)
> +			echo "Kdump is not operational"
> +			EXIT_CODE=3
> +			;;
> +		  2)
> +			echo "Kdump is unsupported on this kernel"
> +			EXIT_CODE=3
> +			;;
> +		esac
> +		exit $EXIT_CODE
>  		;;
> -	1)
> -		echo "Kdump is not operational"
> -		EXIT_CODE=3
> +	  restart)
> +		stop
> +		start
> +		;;
> +	  condrestart)
>  		;;
> -	2)
> -		echo "Kdump is unsupported on this kernel"
> -		EXIT_CODE=3
> +	  propagate)
> +		propagate_ssh_key
>  		;;
> +	  *)
> +		echo $"Usage: $0 {start|stop|status|restart|propagate}"
> +		exit 1
>  	esac
> -	exit $EXIT_CODE
> -	;;
> -  restart)
> -	stop
> -	start
> -	;;
> -  condrestart)
> -	;;
> -  propagate)
> -	propagate_ssh_key
> -	;;
> -  *)
> -	echo $"Usage: $0 {start|stop|status|restart|propagate}"
> -	exit 1
> -esac
> +}
> +
> +# Other kdumpctl instances will block in queue, until this one exits
> +single_instance_lock
> +
> +# To avoid fd 9 leaking, we invoke a subshell, close fd 9 and call main.
> +# So that fd isn't leaking when main is invoking a subshell.
> +(exec 9<&-; main $1)
>  
>  exit $?
> -- 
> 1.8.3.1
> 
> _______________________________________________
> kexec mailing list
> kexec at lists.fedoraproject.org
> https://lists.fedoraproject.org/mailman/listinfo/kexec