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
| ||
|
Date: Tue, 24 Jan 2017 10:34:37 -0800 From: "Darrick J. Wong" <darrick.wong@...cle.com> To: "Bill O'Donnell" <billodo@...hat.com> Cc: Colin Ian King <colin.king@...onical.com>, Eric Sandeen <sandeen@...deen.net>, linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag On Tue, Jan 24, 2017 at 09:04:57AM -0600, Bill O'Donnell wrote: > On Fri, Jan 20, 2017 at 11:04:42PM +0000, Colin Ian King wrote: > > On 20/01/17 20:47, Darrick J. Wong wrote: > > > On Fri, Jan 20, 2017 at 01:26:12PM -0600, Eric Sandeen wrote: > > >> On 1/20/17 8:26 AM, Colin King wrote: > > >>> From: Colin Ian King <colin.king@...onical.com> > > >>> > > >>> If pag cannot be allocated, the current error exit path will trip > > >>> a null pointer deference error when calling xfs_buf_hash_destroy > > >>> with a null pag. Fix this by adding a new error exit lable and > > >>> jumping to this, avoiding the hash destroy and unnecessary kmem_free > > >>> on pag. > > >>> > > >>> Fixes CoverityScan CID#1397628 ("Dereference after null check") > > >>> > > >>> Signed-off-by: Colin Ian King <colin.king@...onical.com> > > >> > > >> Hm, I think this leaves the code with issues. > > >> > > >>> --- > > >>> fs/xfs/xfs_mount.c | 3 ++- > > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > >>> index 9b9540d..4e66cd19 100644 > > >>> --- a/fs/xfs/xfs_mount.c > > >>> +++ b/fs/xfs/xfs_mount.c > > >>> @@ -207,7 +207,7 @@ xfs_initialize_perag( > > >>> > > >>> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL); > > >>> if (!pag) > > >>> - goto out_unwind; > > >>> + goto out_unwind_pags; > > >> > > >> So let's say we got to index == 3 at the top of the loop, and > > >> this fails. > > >> > > >> We succeeded in initializing 0, 1, and 2, but 3 failed. > > >> > > >> So we go to out_unwind_pags with index == 3... > > >> > > >>> pag->pag_agno = index; > > >>> pag->pag_mount = mp; > > >>> spin_lock_init(&pag->pag_ici_lock); > > >>> @@ -242,6 +242,7 @@ xfs_initialize_perag( > > >>> out_unwind: > > >>> xfs_buf_hash_destroy(pag); > > >>> kmem_free(pag); > > >>> +out_unwind_pags: > > >> > > >> ... where index == 3, and: > > >> > > >>> for (; index > first_initialised; index--) { > > >>> pag = radix_tree_delete(&mp->m_perag_tree, index); > > >> > > >> this should fail, because it never got inserted, and... > > >> > > >>> xfs_buf_hash_destroy(pag); > > >> > > >> this still tries to destroy a NULL pag, no? > > >> > > >> There also seems to be an existing issue w/the code where ag 0 is > > >> never torn down in the error case, because first_initialized doesn't > > >> stay set to 0: > > >> > > >> if (!first_initialised) > > >> first_initialised = index; > > >> > > >> And we don't even tear down ag 1, because: > > >> > > >>> for (; index > first_initialised; index--) { > > >>> pag = radix_tree_delete(&mp->m_perag_tree, index); > > >> > > >> when the loop reaches the first initialized AG, it stops. > > >> > > >> So we seem to always leak at least 2 if we managed to get far enough > > >> to initialize them. > > > > > > Ugh, yeah, the the whole error exit from that function is fubar... > > > Anyone want to clean this up? > > > > I may step back on this if somebody else wants to fix this up properly. > > I'll take it. Acknowledged. :) --D > -Bill > > > > > > > > > > --D > > > > > >> > > >> -Eric > > >> > > >>> > > >> -- > > >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > >> the body of a message to majordomo@...r.kernel.org > > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@...r.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists