linux-kernel - Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs

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:	Wed, 11 Apr 2012 21:47:53 +0300
From:	"Rémi Denis-Courmont" <remi@...lab.net>
To:	Mauro Carvalho Chehab <mchehab@...hat.com>
Cc:	mchehab@...radead.org, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs

	Hello,

Le mercredi 11 avril 2012 20:02:00 Mauro Carvalho Chehab, vous avez écrit :
> Using unsigned instead of enum is not a good idea, from API POV, as
> unsigned has different sizes on 32 bits and 64 bits.

Fair enough. But then we can do that instead:
typedef XXX __enum_t;
where XXX is the unsigned integer with the right number of bits. Since Linux 
does not use short enums, this ought to work fine.

> Yet, using enum was really a very bad idea, and, on all new stuff,
> we're not accepting any new enum field.

That is unfortunately not true. You do follow that rule for new fields to 
existing V4L2 structure. But you have been royally ignoring that rule when it 
comes to extending existing enumerations:

linux-media does regularly add new enum values to existing enums. That is new 
stuff too, and every single time you do that, you do BREAK THE USERSPACE ABI. 
This is entirely unacceptable and against established kernel development 
policies.

For instance, in Linux 3.1, V4L2_CTRL_TYPE_BITMASK was added. This broke 
userspace. And there are some pending patches adding more of the same thing... 
And V4L2_MEMORY_DMABUF will similarly break the user-to-kernel interface, 
which is yet worse.

> That's said, a patch like that were proposed in the past. See:
> 	http://www.spinics.net/lists/vfl/msg25686.html
> 
> Alan said there [1] that:
> 	"Its a fundamental change to a public structure from enum to u32. I think
> you are right but this change seems too late by several years."

I cannot see how the kernel is protected against userspace injecting error 
values into enums. For all I know, depending how the kernel is compiled, 
userspace might be able to trigger "undefined" behavior in kernel space.

Is it be several years too late to fix a bug?

(...)
> I don't think anything has changed since then that would make it easier
> to apply a change like that.

OK. But then you cannot update extend existing enums... No DMA buffers, no 
integer menu controls...

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis
--
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/