linux-kernel - Re: [PATCH 1/2] dmaengine: shdma: fix locking

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: <20110531065051.GA16157@linux-sh.org>
Date:	Tue, 31 May 2011 15:50:51 +0900
From:	Paul Mundt <lethal@...ux-sh.org>
To:	Guennadi Liakhovetski <g.liakhovetski@....de>
Cc:	linux-sh@...r.kernel.org, linux-kernel@...r.kernel.org,
	Dan Williams <dan.j.williams@...el.com>,
	Simon Horman <horms@...ge.net.au>,
	Magnus Damm <damm@...nsource.se>
Subject: Re: [PATCH 1/2] dmaengine: shdma: fix locking

On Fri, Apr 29, 2011 at 07:09:21PM +0200, Guennadi Liakhovetski wrote:
>  static int sh_dmae_rst(struct sh_dmae_device *shdev)
>  {
..
> +	dmaor_write(shdev, dmaor | shdev->pdata->dmaor_init);

On Fri, Apr 29, 2011 at 07:09:25PM +0200, Guennadi Liakhovetski wrote:
> +static int sh_dmae_runtime_resume(struct device *dev)
> +{
> +	struct sh_dmae_device *shdev = dev_get_drvdata(dev);
> +
> +	return sh_dmae_rst(shdev);
..

Yet in sh_dmae_probe() we have:

        shdev->pdata = pdata;

        pm_runtime_enable(&pdev->dev);
        pm_runtime_get_sync(&pdev->dev);

..

        /* reset dma controller - only needed as a test */
        err = sh_dmae_rst(shdev);
        if (err)
                goto rst_err;

..

        pm_runtime_put(&pdev->dev);

        platform_set_drvdata(pdev, shdev);
        dma_async_device_register(&shdev->common);

        return err;
..

So I'm wondering how this was ever actually tested. The original sh_dmae_rst()
call is safe due to passing along the shdev pointer with pdata initialized
explicitly, while the runtime PM bits fetch the pointer via dev_get_drvdata()
at a time where drvdata hasn't even been initialized yet, resulting in a rather
predictable oops:

	Unable to handle kernel NULL pointer dereference at virtual address 000000c4
	pc = 8025adee
	*pde = 00000000
	Oops: 0000 [#1]
	Modules linked in:

	Pid : 1, Comm:           swapper
	CPU : 0                  Not tainted  (3.0.0-rc1-00012-g9436b4a-dirty #1456)

	PC is at sh_dmae_rst+0x28/0x86
	PR is at sh_dmae_rst+0x22/0x86
	PC  : 8025adee SP  : 9e803d10 SR  : 400080f1 TEA : 000000c4
	R0  : 000000c4 R1  : 0000fff8 R2  : 00000000 R3  : 00000040
	R4  : 000000f0 R5  : 00000000 R6  : 00000000 R7  : 804f184c
	R8  : 00000000 R9  : 804dd0e8 R10 : 80283204 R11 : ffffffda
	R12 : 000000a0 R13 : 804dd18c R14 : 9e803d10
	MACH: 00000000 MACL: 00008f20 GBR : 00000000 PR  : 8025ade8

	Call trace:
	[<8025ae70>] sh_dmae_runtime_resume+0x24/0x34
	[<80283238>] pm_generic_runtime_resume+0x34/0x3c
	[<80283370>] rpm_callback+0x4a/0x7e
	[<80283efc>] rpm_resume+0x240/0x384
	[<80283f54>] rpm_resume+0x298/0x384
	[<8028428c>] __pm_runtime_resume+0x44/0x7c
	[<8038a358>] __ioremap_caller+0x0/0xec
	[<80284296>] __pm_runtime_resume+0x4e/0x7c
	[<8038a358>] __ioremap_caller+0x0/0xec
	[<80666254>] sh_dmae_probe+0x180/0x6a0
	[<802803ae>] platform_drv_probe+0x26/0x2e

I've fixed this up now, but I am growing rather weary of applying anything with
runtime PM in the subject that alleges to have been tested.

The next runtime PM patch that doesn't even boot will be immediately reverted
and have a kernel version or two to sit things out in order to try to get
things in demonstrable functional order.
--
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/