linux-kernel - Re: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated structure is transferred from userspace to kernelspace. Keep the old ioctl around for compatibility so that existing code is not broken.

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

Date:	Mon, 23 May 2011 17:32:23 +0200
From:	Andreas Oberritter <obi@...uxtv.org>
To:	Hans Petter Selasky <hselasky@....net>
CC:	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	Mauro Carvalho Chehab <mchehab@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated
 structure is transferred from userspace to kernelspace. Keep the old ioctl
 around for compatibility so that existing code is not broken.

On 05/23/2011 04:51 PM, Hans Petter Selasky wrote:
> On Monday 23 May 2011 16:37:18 Andreas Oberritter wrote:
>> On 05/23/2011 03:58 PM, Hans Petter Selasky wrote:
>>> From be7d0f72ebf4d945cfb2a5c9cc871707f72e1e3c Mon Sep 17 00:00:00 2001
>>> From: Hans Petter Selasky <hselasky@....net>
>>> Date: Mon, 23 May 2011 15:56:31 +0200
>>> Subject: [PATCH] FE_GET_PROPERTY should be _IOW, because the associated
>>> structure is transferred from userspace to kernelspace. Keep the old
>>> ioctl around for compatibility so that existing code is not broken.
>>
> 
> Hi,
> 
>> Good catch, but I think _IOWR would be right, because the result gets
>> copied from kernelspace to userspace.
> 
> Those flags are only for the IOCTL associated structure itself. The V4L DVB 
> kernel only reads the dtv_properties structure in either case and does not 
> write any data back to it. That's why only _IOW is required.

I see.

> I checked somewhat and the R/W bits in the IOCTL command does not appear do be 
> matched to the R/W permissions you have on the file handle? Or am I mistaken?

You're right. There's no direct relationship between them, at least not
within dvb-core.

> In other words the IOCTL R/W (_IOC_READ, _IOC_WRITE) bits should not reflect 
> what the IOCTL actually does, like modifying indirect data?

I'm not sure. Your patch is certainly doing the right thing for the
current implementation of dvb_usercopy, which however wasn't designed
with variable length arrays in mind.

Taking dvb_usercopy aside, my interpretation of the ioctl bits was:
- _IOC_READ is required if copy_to_user/put_user needs to be used during
the ioctl.
- _IOC_WRITE is required if copy_from_user/get_user needs to be used
during the ioctl.

Whether that's limited to the structure directly encoded in the ioctl or
not is unclear to me. Maybe someone at LKML can shed some light on that.

Regards,
Andreas
--
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/