linux-kernel - Re: [PATCH] mm/swap: abort swapoff after disk error

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: <alpine.LNX.2.00.1212171953070.5927@eggly.anvils>
Date:	Mon, 17 Dec 2012 20:24:36 -0800 (PST)
From:	Hugh Dickins <hughd@...gle.com>
To:	Konstantin Khlebnikov <khlebnikov@...nvz.org>
cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>
Subject: Re: [PATCH] mm/swap: abort swapoff after disk error

On Fri, 14 Dec 2012, Konstantin Khlebnikov wrote:

> Content of non-uptodate pages completely random, we cannot expose them into
> userspace. This leads to information leak and will crash userspace for sure.

Good find, yes, it's very wrong as is.  But, sorry, I don't like your fix
- better than ignoring the issue as at present, but not the right answer.

> Probably we can reuse hwpoison entries here, but tmpfs already too complex.

HWpoison entries?  They're for when that page of RAM is bad, but this is
quite a different case: the page is fine and can perfectly well be freed
and reused - what's bad is the data currently in it.

> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@...nvz.org>
> Original-patch-by: Alexey Kuznetsov <kuznet@....inr.ac.ru>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Hugh Dickins <hughd@...gle.com>
> Cc: Andi Kleen <andi@...stfloor.org>
> ---
>  mm/swapfile.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index e97a0e5..98fc2fd 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1127,6 +1127,22 @@ int try_to_unuse(unsigned int type, bool frontswap,
>  		wait_on_page_writeback(page);
>  
>  		/*
> +		 * If read failed we cannot map not-uptodate page to
> +		 * user space. Actually, we are in serious troubles,
> +		 * we do not even know what process to kill. So, the only

try_to_unuse() is all about locating exactly where this page belongs;
and if the user is lucky, the page in question won't even be needed again
before the process exits, so nothing should be killed at this point.

> +		 * variant remains: to stop swapoff() and allow someone
> +		 * to kill processes to zap invalid pages.

No, we should not abort swapoff: there's every reason to continue,
to make sure that this unreliable area can be taken out of service.

> +		 *
> +		 * TODO replace page with hwpoison entry in pte and shmem.

Instead of blindly going ahead and inserting ptes pointing to the
!PageUptodate page, unuse_pte() and shmem_unuse_inode() should insert
a substitute bad swapentry, to generate SIGBUS if it's accessed.

swp_entry(1, 0) might serve, but there's probably a few mods needed
here and there; and getting the details right (e.g. memcg charges)
will need care.

Not as straightforward as your block below, I admit.  I wonder if you
posted that just to stir me to do better: or can you take it further?

Thanks,
Hugh

> +		 */
> +		if (unlikely(!PageUptodate(page))) {
> +			unlock_page(page);
> +			page_cache_release(page);
> +			retval = -EIO;
> +			break;
> +		}
> +
> +		/*
>  		 * Remove all references to entry.
>  		 */
>  		swcount = *swap_map;
--
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/