linux-kernel - Re: Bug 16061 - single stepping in a signal handler can cause the single step flag to get "stuck"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100602200720.GA28062@redhat.com>
Date:	Wed, 2 Jun 2010 22:07:20 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Roland McGrath <roland@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Evan Teran <eteran@...m.rit.edu>,
	Jan Kratochvil <jan.kratochvil@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Bug 16061 - single stepping in a signal handler can cause the
	single step flag to get "stuck"

sorry for noise, forgot to mention...

On 06/02, Oleg Nesterov wrote:
>
> However, what I am thinking about is the more "clever" change (it
> passed ptrace-tests).
>
> Do you think it can be correct? I am asking because I never understood
> the TIF_SINGLESTEP/TIF_FORCED_TF interaction. But otoh, shouldn't
> TIF_FORCED_TF + X86_EFLAGS_TF always imply TIF_SINGLESTEP? at least
> in handle_signal().
>
> IOW, help! To me, the patch below is also cleanup. But only if you think
> it can fly ;)

and it is not clear to me if we should keep this code

	/*
	 * Clear TF when entering the signal handler, but
	 * notify any tracer that was single-stepping it.
	 * The tracer may want to single-step inside the
	 * handler too.
	 */
	regs->flags &= ~X86_EFLAGS_TF;

in handle_signal(). If TF was set by us, it was cleared by
user_disable_single_step(). Otherwise, why should we clear it if
the tracer did set_flags(X86_EFLAGS_TF) ?

Oleg.

> --- 34-rc1/arch/x86/kernel/signal.c~BZ16061_MAYBE_FIX	2010-06-02 21:11:07.000000000 +0200
> +++ 34-rc1/arch/x86/kernel/signal.c	2010-06-02 21:11:48.000000000 +0200
> @@ -682,6 +682,7 @@ static int
>  handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
>  	      sigset_t *oldset, struct pt_regs *regs)
>  {
> +	bool stepping;
>  	int ret;
>  
>  	/* Are we from a system call? */
> @@ -706,13 +707,10 @@ handle_signal(unsigned long sig, siginfo
>  		}
>  	}
>  
> -	/*
> -	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
> -	 * flag so that register information in the sigcontext is correct.
> -	 */
> -	if (unlikely(regs->flags & X86_EFLAGS_TF) &&
> -	    likely(test_and_clear_thread_flag(TIF_FORCED_TF)))
> -		regs->flags &= ~X86_EFLAGS_TF;
> +	stepping = test_thread_flag(TIF_SINGLESTEP);
> +	if (stepping)
> +		// do this before setup_sigcontext()
> +		user_disable_single_step(current);
>  
>  	ret = setup_rt_frame(sig, ka, info, oldset, regs);
>  
> @@ -748,8 +746,7 @@ handle_signal(unsigned long sig, siginfo
>  	recalc_sigpending();
>  	spin_unlock_irq(&current->sighand->siglock);
>  
> -	tracehook_signal_handler(sig, info, ka, regs,
> -				 test_thread_flag(TIF_SINGLESTEP));
> +	tracehook_signal_handler(sig, info, ka, regs, stepping);
>  
>  	return 0;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/