linux-kernel - Re: [PATCH 2/2] Staging: android: ashmem: Add support for 32bit ashmem calls in a 64bit kernel

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: <20121204114511.GI6568@mwanda>
Date:	Tue, 4 Dec 2012 14:45:11 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Serban Constantinescu <serban.constantinescu@....com>
Cc:	gregkh@...uxfoundation.org, arve@...roid.com,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	john.stultz@...aro.org, ccross@...roid.com,
	zach.pfeffer@...aro.org, Dave.Butcher@....com
Subject: Re: [PATCH 2/2] Staging: android: ashmem: Add support for 32bit
 ashmem calls in a 64bit kernel

I don't understand this, and I'm going to embarrass myself by
displaying my ignorance for all to see.  Why is this code so
different from all the other 32 bit compat code that we have in the
kernel?

On Tue, Dec 04, 2012 at 10:44:14AM +0000, Serban Constantinescu wrote:
> -static int set_name(struct ashmem_area *asma, void __user *name)
> +static int set_name(struct ashmem_area *asma, userptr32_t name)

The user passes in a value which is a 32 pointer.  ashmem_ioctl()
accepts it as "unsigned long arg".  We pass it to set_name() which
truncates away the high zeros so now its a u32 (userptr32_t).  We
then cast it to (unsigned long) and then we cast it to a void
pointer.

What's the point?  Why not just take"unsigned long arg" and cast it
to a pointer directly?

>  	if (unlikely(copy_from_user(asma->name + ASHMEM_NAME_PREFIX_LEN,
> -				    name, ASHMEM_NAME_LEN)))
> +				    (void *)(unsigned long)name, ASHMEM_NAME_LEN)))
>  		ret = -EFAULT;

This will introduce a Sparse complaint.  It should be:
				(void __user *)(unsigned long)name.

But actually we shouldn't need to do this casting.  Any casting
which we need to do should be done in one place instead of pushed
out to every function.

> +	switch (_IOC_NR(cmd)) {
> +	case _IOC_NR(ASHMEM_SET_NAME):
> +		if (_IOC_SIZE(cmd) != sizeof(char[ASHMEM_NAME_LEN]))
> +			pr_err("ashmem: ASHMEM_SET_NAME transaction size differs\n");

Don't merge debug code into the kernel.  It just means people can
spam /var/log/messages.

regards,
dan carpenter

--
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/