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

Message-ID: <4DE6ABF5.6020008@redhat.com>
Date:	Wed, 01 Jun 2011 18:15:33 -0300
From:	Mauro Carvalho Chehab <mchehab@...hat.com>
To:	Andreas Oberritter <obi@...uxtv.org>
CC:	Hans Petter Selasky <hselasky@....net>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	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.

Em 23-05-2011 12:32, Andreas Oberritter escreveu:
> 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.

The dvb_usercopy will do the right thing, if we use _IOR or _IORW.

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

That is my understanding too. I agree that _IOWR seems to be the more appropriate
definition for it.

That's said, this is just a naming convention. Kernel core won't enforce
any special behavior, as there are some violations about this convention
on a few places.

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

I prefer to not apply this patch, as it won't fix anything. Adding an _OLD means
that we'll need later to remove it, causing a regression. Ok, we may do like we did
with V4L _OLD ioctl's that were marked as _OLD at 2.6.5 and were removed on a late
2.6.3x.

Cheers,
Mauro
--
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/