linux-kernel - Re: [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser

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] [day] [month] [year] [list]

Message-ID: <20130123174046.GD15627@herton-Z68MA-D2H-B3>
Date:	Wed, 23 Jan 2013 15:40:47 -0200
From:	Herton Ronaldo Krzesinski <herton.krzesinski@...onical.com>
To:	Ilija Hadzic <ilijahadzic@...il.com>
Cc:	Shuah Khan <shuah.khan@...com>,
	Greg KH <gregkh@...uxfoundation.org>, jglisse@...hat.com,
	dri-devel@...ts.freedesktop.org,
	LKML <linux-kernel@...r.kernel.org>,
	"Deucher, Alexander" <alexander.deucher@....com>,
	shuahkhan@...il.com, airlied@...hat.com
Subject: Re: [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in
 radeon_cs_parser_fini()

On Wed, Jan 23, 2013 at 12:21:29PM -0500, Ilija Hadzic wrote:
> On Wed, Jan 23, 2013 at 11:07 AM, Herton Ronaldo Krzesinski
> <herton.krzesinski@...onical.com> wrote:
> > On Thu, Jan 17, 2013 at 10:09:42AM -0700, Shuah Khan wrote:
> >> On Wed, 2013-01-16 at 21:06 -0600, Ilija Hadzic wrote:
> >> > Actually, the code path affected by your patch is not executed in UMS mode
> >> > at all. Notice that radeon_cs_parser_fini is only called from
> >> > radeon_cs_ioctl which is a KMS-only ioctl (see radeon_kms.c).
> >> >
> >> > The equivalent of the fix you are trying to do is in
> >> > a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e (function patched by that one is
> >> > the one used by legacy-CS ioctl), which you should go together
> >> > with ff4bd0827764e10a428a9d39e6814c5478863f94 if you are backporting UMS
> >> > fixes to 3.7. Both are needed to prevent kernel crashes in UMS mode.
> >> >
> >> > -- Ilija
> >>
> >> Thanks. I will take a look at a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e.
> >> I sent back-ported ff4bd0827764e10a428a9d39e6814c5478863f94 patch to
> >> stable and I will back-port and send
> >> a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e as well.
> >
> > While at it, also looks like commit
> > 25d8999780f8c1f53928f4a24a09c01550423109 could also be added to stables.
> > But while looking at it, while the patch itself is correct, I noted that
> > there is a possibility of double free: if either of the
> > p->chunks[p->chunk_ib_idx].kpage[] items are non NULL, we will free it in
> > radeon_cs_parser_init and also when calling one of the fini functions
> > (radeon_cs_parser_fini or r600_cs_parser_fini). So either we need to set
> > kpage[n] to NULL after kfree, or just return the error.
> >
> 
> Yes you are right. The error-handling path should force both kpage[]
> pointers to NULL before returning -ENOMEM. That would fix the double
> free.
> 
> Regarding, inclusion of 25d8999780f8c1f53928f4a24a09c01550423109 into
> stable (and possible follow-up patch to fix potential double-kfree),
> does this pass the "no fixes for theoretical problems in stable"
> criterion ?
> 
> It is an obvious bug, but it happens so rarely that I am not surprised
> that it has never happened: You need an (old) AGP card *and* you need
> to run out of kmalloc memory.

About the "theoretical problem" criteria, I think it applies more to
race conditions not detected in practice, things that are not obvious/
deterministic etc., but indeed commit 25d89997 looks not be worth to be
added on stables.

> 
> -- Ilija
> 

-- 
[]'s
Herton
--
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/