linux-kernel - Re: [PATCH V2] drivers/uio/uio.c: DMA mapping, interrupt extensions, etc.

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]

Date:	Sat, 17 Apr 2010 12:43:09 +0200
From:	Joerg Roedel <joro@...tes.org>
To:	Tom Lyon <pugs@...co.com>
Cc:	mst@...hat.com, hjk@...utronix.de, gregkh@...e.de,
	chrisw@...s-sol.org, avi@...hat.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] drivers/uio/uio.c: DMA mapping, interrupt
	extensions, etc.

On Thu, Apr 15, 2010 at 01:55:29PM -0700, Tom Lyon wrote:

> +	down(&idev->gate);
> +	if (idev->listeners == 0) {		/* first open */
> +		if (idev->pmaster && !iommu_found() && !capable(CAP_SYS_RAWIO)) {
> +			up(&idev->gate);
> +			return -EPERM;
> +		}
> +		/* reset to known state if we can */
> +		if (idev->pdev)
> +			(void) pci_reset_function(idev->pdev);
> +	}
> +	idev->listeners++;
> +	up(&idev->gate);

Why do you want to allow multiple opens for a device? Is it not
sufficient to allow only one?


> +	if (idev->domain == NULL) {
> +		if (!list_empty(&listener->dm_list))	/* no mix with anywhere */
> +			return -EINVAL;
> +		if (!iommu_found())
> +			return -EINVAL;
> +		idev->domain = iommu_domain_alloc();
> +		if (idev->domain == NULL)
> +			return -ENXIO;
> +		idev->cachec = iommu_domain_has_cap(idev->domain,
> +					IOMMU_CAP_CACHE_COHERENCY);
> +		ret = iommu_attach_device(idev->domain, idev->dev->parent);
> +		if (ret) {
> +			iommu_domain_free(idev->domain);
> +			idev->domain = NULL;
> +			printk(KERN_ERR "%s: device_attach failed %d\n", __func__, ret);
> +			return ret;
> +		}
> +	}

If userspace calls this path this will make all the addresses mapped
with DMA-API paths unusable by the device. This doesn't look like a sane
userspace interface.

For better and more in-depth review I suggest that you split up this
large patch into a series of smaler which implement specific aspects of
your work.

	Joerg

P.S.: I got these warning when applying your patches ...

Applying: drivers/uio/uio_pci_generic.c: allow access for non-privileged processes
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:86: trailing whitespace.
                        info->mem[j].name = name; 
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:87: trailing whitespace.
                        info->mem[j].addr = pci_resource_start(pdev, i); 
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:89: trailing whitespace.
                        info->mem[j].memtype = UIO_MEM_PHYS; 
warning: 3 lines add whitespace errors.
Applying: drivers/uio/uio.c: DMA mapping, interrupt extensions, etc.
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:315: trailing whitespace.
                ret = iommu_map_range(idev->domain, iova, 
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:325: trailing whitespace.
        } 
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:366: trailing whitespace.
         * adjacent pages, but noone seems to really do that. So we squash 
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:368: trailing whitespace.
         * This works if (a) there is an iommu, or (b) the user allocates 
/home/joro/src/linux.trees.git/.git/rebase-apply/patch:578: trailing whitespace.
                        unsigned int cmd, unsigned long arg) 
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.

And there are also some coding-style issues.
--
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/