linux-kernel - Re: [PATCH 01/10] nbd: Fix timeout detection

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1443400064.2517.16.camel@decadent.org.uk>
Date:	Mon, 28 Sep 2015 01:27:44 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Markus Pargmann <mpa@...gutronix.de>, Jens Axboe <axboe@...nel.dk>
Cc:	linux-kernel@...r.kernel.org, nbd-general@...ts.sourceforge.net,
	kernel@...gutronix.de, Michal Belczyk <belczyk@....krakow.pl>,
	Hermann Lauer <Hermann.Lauer@....uni-heidelberg.de>
Subject: Re: [PATCH 01/10] nbd: Fix timeout detection

On Mon, 2015-08-17 at 08:20 +0200, Markus Pargmann wrote:
> At the moment the nbd timeout just detects hanging tcp operations. This
> is not enough to detect a hanging or bad connection as expected of a
> timeout.
> 
> This patch redesigns the timeout detection to include some more cases.
> The timeout is now in relation to replies from the server. If the server
> does not send replies within the timeout the connection will be shut
> down.
> 
> The patch adds a continous timer 'timeout_timer' that is setup in one of
> two cases:
>  - The request list is empty and we are sending the first request out to
>    the server. We want to have a reply within the given timeout,
>    otherwise we consider the connection to be dead.
>  - A server response was received. This means the server is still
>    communicating with us. The timer is reset to the timeout value.
> 
> The timer is not stopped if the list becomes empty. It will just trigger
> a timeout which will directly leave the handling routine again as the
> request list is empty.
> 
> The whole patch does not use any additional explicit locking. The
> list_empty() calls are safe to be used concurrently. The timer is locked
> internally as we just use mod_timer and del_timer_sync().

This is crazy.  The timer is locked internally but the tasks are not.
So it is possible for the timeout handler to kill a task after it
exited from nbd_do_it()/nbd_thread_recv(), or after it exited entirely
(use-after-free).

[...]
> +> 	> task = READ_ONCE(nbd->task_send);
> +> 	> if (task)
> +> 	> 	> force_sig(SIGKILL, nbd->task_send);
[...]

And this is just... what?  What is the point of using READ_ONCE() if
you're going to look up nbd->task_send again?

Ben.

-- 
Ben Hutchings
All extremists should be taken out and shot.
Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)