[RFC] v4l2: use unsigned rather than enums in ioctl() structs

  With an enumeration, the compiler assumes that the integer value is one
allowed by the underlying enumeration type. With optimization enabled
this can result in byte code that is unable to cope with other values.
For instance, GCC can compile a switch() block using a "jump table" to
avoid repetitive conditional branching.

This can be a problem both from user to kernel and kernel to user.

* Evil user space can pass error values to the kernel via system
calls. There are no sane ways to protect the kernel: the compiler may
optimize away range checks if it deems that all legitimate values are
within the range.

* The kernel can break the user space binary interface whenever a new
value is added to an existing enumeration. A newer kernel can then
return an enumerated value that was not allowed by older kernel headers
against which the user program was compiled. In principles, V4L2 user
space needs to be recompiled whenever videodev2.h has updated enums...
(This an obvious problem with V4L2_QUERYCTRL.)

Fortunately, the Linux ABI disables short-enums on all platforms. Even
the Linux variant of the ARM AAPCS contradicts the standard ARM AAPCS
in doing so.

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 include/linux/videodev2.h |   42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)
  

Em 05-04-2012 14:52, Rémi Denis-Courmont escreveu:
> With an enumeration, the compiler assumes that the integer value is one
> allowed by the underlying enumeration type. With optimization enabled
> this can result in byte code that is unable to cope with other values.
> For instance, GCC can compile a switch() block using a "jump table" to
> avoid repetitive conditional branching.
> 
> This can be a problem both from user to kernel and kernel to user.
> 
> * Evil user space can pass error values to the kernel via system
> calls. There are no sane ways to protect the kernel: the compiler may
> optimize away range checks if it deems that all legitimate values are
> within the range.
> 
> * The kernel can break the user space binary interface whenever a new
> value is added to an existing enumeration. A newer kernel can then
> return an enumerated value that was not allowed by older kernel headers
> against which the user program was compiled. In principles, V4L2 user
> space needs to be recompiled whenever videodev2.h has updated enums...
> (This an obvious problem with V4L2_QUERYCTRL.)
> 
> Fortunately, the Linux ABI disables short-enums on all platforms. Even
> the Linux variant of the ARM AAPCS contradicts the standard ARM AAPCS
> in doing so.


Using unsigned instead of enum is not a good idea, from API POV, as unsigned
has different sizes on 32 bits and 64 bits. Yet, using enum was really a very
bad idea, and, on all new stuff, we're not accepting any new enum field.

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 also didn't like it, because of the same reasons.

[1] http://www.spinics.net/lists/vfl/msg25687.html

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

Well, eventually, a compat code at the v4l2-ioctl.c might be written to translate
a call if the enum size is different than the integer size.

Regards,
Mauro

> 
> Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> ---
>  include/linux/videodev2.h |   42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index c9c9a46..df3b8f0 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -292,10 +292,10 @@ struct v4l2_pix_format {
>  	__u32         		width;
>  	__u32			height;
>  	__u32			pixelformat;
> -	enum v4l2_field  	field;
> +	unsigned		field;
>  	__u32            	bytesperline;	/* for padding, zero if unused */
>  	__u32          		sizeimage;
> -	enum v4l2_colorspace	colorspace;
> +	unsigned		colorspace;
>  	__u32			priv;		/* private data, depends on pixelformat */
>  };
>  
> @@ -432,7 +432,7 @@ struct v4l2_pix_format {
>   */
>  struct v4l2_fmtdesc {
>  	__u32		    index;             /* Format number      */
> -	enum v4l2_buf_type  type;              /* buffer type        */
> +	unsigned	    type;              /* buffer type        */
>  	__u32               flags;
>  	__u8		    description[32];   /* Description string */
>  	__u32		    pixelformat;       /* Format fourcc      */
> @@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
>   */
>  struct v4l2_requestbuffers {
>  	__u32			count;
> -	enum v4l2_buf_type      type;
> -	enum v4l2_memory        memory;
> +	unsigned		type;
> +	unsigned		memory;
>  	__u32			reserved[2];
>  };
>  
> @@ -636,16 +636,16 @@ struct v4l2_plane {
>   */
>  struct v4l2_buffer {
>  	__u32			index;
> -	enum v4l2_buf_type      type;
> +	unsigned		type;
>  	__u32			bytesused;
>  	__u32			flags;
> -	enum v4l2_field		field;
> +	unsigned		field;
>  	struct timeval		timestamp;
>  	struct v4l2_timecode	timecode;
>  	__u32			sequence;
>  
>  	/* memory location */
> -	enum v4l2_memory        memory;
> +	unsigned		memory;
>  	union {
>  		__u32           offset;
>  		unsigned long   userptr;
> @@ -708,7 +708,7 @@ struct v4l2_clip {
>  
>  struct v4l2_window {
>  	struct v4l2_rect        w;
> -	enum v4l2_field  	field;
> +	unsigned		field;
>  	__u32			chromakey;
>  	struct v4l2_clip	__user *clips;
>  	__u32			clipcount;
> @@ -745,14 +745,14 @@ struct v4l2_outputparm {
>   *	I N P U T   I M A G E   C R O P P I N G
>   */
>  struct v4l2_cropcap {
> -	enum v4l2_buf_type      type;
> +	unsigned		type;
>  	struct v4l2_rect        bounds;
>  	struct v4l2_rect        defrect;
>  	struct v4l2_fract       pixelaspect;
>  };
>  
>  struct v4l2_crop {
> -	enum v4l2_buf_type      type;
> +	unsigned		type;
>  	struct v4l2_rect        c;
>  };
>  
> @@ -1156,7 +1156,7 @@ enum v4l2_ctrl_type {
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>  struct v4l2_queryctrl {
>  	__u32		     id;
> -	enum v4l2_ctrl_type  type;
> +	unsigned	     type;
>  	__u8		     name[32];	/* Whatever */
>  	__s32		     minimum;	/* Note signedness */
>  	__s32		     maximum;
> @@ -1788,7 +1788,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  struct v4l2_tuner {
>  	__u32                   index;
>  	__u8			name[32];
> -	enum v4l2_tuner_type    type;
> +	unsigned		type;
>  	__u32			capability;
>  	__u32			rangelow;
>  	__u32			rangehigh;
> @@ -1838,14 +1838,14 @@ struct v4l2_modulator {
>  
>  struct v4l2_frequency {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	unsigned	      type;
>  	__u32		      frequency;
>  	__u32		      reserved[8];
>  };
>  
>  struct v4l2_hw_freq_seek {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	unsigned	      type;
>  	__u32		      seek_upward;
>  	__u32		      wrap_around;
>  	__u32		      spacing;
> @@ -2056,7 +2056,7 @@ struct v4l2_sliced_vbi_cap {
>  				 (equals frame lines 313-336 for 625 line video
>  				  standards, 263-286 for 525 line standards) */
>  	__u16   service_lines[2][24];
> -	enum v4l2_buf_type type;
> +	unsigned type;
>  	__u32   reserved[3];    /* must be 0 */
>  };
>  
> @@ -2146,8 +2146,8 @@ struct v4l2_pix_format_mplane {
>  	__u32				width;
>  	__u32				height;
>  	__u32				pixelformat;
> -	enum v4l2_field			field;
> -	enum v4l2_colorspace		colorspace;
> +	unsigned			field;
> +	unsigned			colorspace;
>  
>  	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
>  	__u8				num_planes;
> @@ -2165,7 +2165,7 @@ struct v4l2_pix_format_mplane {
>   * @raw_data:	placeholder for future extensions and custom formats
>   */
>  struct v4l2_format {
> -	enum v4l2_buf_type type;
> +	unsigned type;
>  	union {
>  		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
>  		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
> @@ -2179,7 +2179,7 @@ struct v4l2_format {
>  /*	Stream type-dependent parameters
>   */
>  struct v4l2_streamparm {
> -	enum v4l2_buf_type type;
> +	unsigned type;
>  	union {
>  		struct v4l2_captureparm	capture;
>  		struct v4l2_outputparm	output;
> @@ -2299,7 +2299,7 @@ struct v4l2_dbg_chip_ident {
>  struct v4l2_create_buffers {
>  	__u32			index;
>  	__u32			count;
> -	enum v4l2_memory        memory;
> +	unsigned		memory;
>  	struct v4l2_format	format;
>  	__u32			reserved[8];
>  };

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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

Em 11-04-2012 15:47, Rémi Denis-Courmont escreveu:
> 	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.

I forgot to comment about that on the last e-mail. 

A solution close to the above one were already proposed:
	http://www.spinics.net/lists/vfl/msg25707.html

There were also another proposal there that might solve:
	http://www.spinics.net/lists/vfl/msg25702.html


Something like:

#if sizeof(enum) == 1
	typedef u8	__enum_t;
#elif sizeof(enum) == 2
	typedef u16	__enum_t;
#elif sizeof(enum) == 4
	typedef u32	__enum_t;
#elif sizeof(enum) == 8
	typedef u64	__enum_t;
#else
	typedef enum __enum_t;
#endif

Can actually work. Not sure if I really like adding a typedef, but maybe
this is the less dirty way to fix it.

We'll need to properly test the v4l2-compat32 code, as it will need 
to handle a different enum size on userspace. So, there, we'll likely
need to replace every enum with just "u32". Hmm... arm with 64 bits
(if/when added) may be an additional issue for the compat stuff.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

2012/4/11 Rémi Denis-Courmont <remi@remlab.net>:
>        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.
>

I agree that breaking user-to-kernel interface is not advised.
We came across a similar problem some years ago with the ALSA sound
kernel drivers.
The solution we used was:
1) If a change is likely to change the user-to-kernel API, add a new
IOCTL for it.
Then old userland software can use the old IOCTL, and new userland
software can use the new IOCTL.
2) Add an version IOCTL that returns the current API level, so that
the app can be written to support more than one API interface,
depending on which kernel it is running on. The version IOCTL simply
returns an u32 value. This is a consistent part of the user-kernel API
that will never change.
3) Add "depreciated" compiler warnings to all the old API IOCTL calls,
so app developers know they should be working to update their apps.
4) After a few years, remove the old IOCTLs.
5) Use "uint32_t" and "uint64_t" types for all IOCTL calls, and not
"unsigned int" or "unsigned long int".
I.e. All structures passed in IOCTLs use fixed bit sized parameters
for everything except of course pointers. Pointers depend on
architecture.
6) Add a #if #endif around the old API, so a user compiling their own
kernel can decide if the old API exists or not. User might want to do
this for security reasons.

Kind Regards

James
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Em 12-04-2012 05:04, James Courtier-Dutton escreveu:
> 2012/4/11 Rémi Denis-Courmont <remi@remlab.net>:
>>        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.
>>
> 
> I agree that breaking user-to-kernel interface is not advised.
> We came across a similar problem some years ago with the ALSA sound
> kernel drivers.
> The solution we used was:
> 1) If a change is likely to change the user-to-kernel API, add a new
> IOCTL for it.
> Then old userland software can use the old IOCTL, and new userland
> software can use the new IOCTL.

V4L2 API has about 80 ioctl's. Add compat code for most of them (as most have
enum's) is not fun.

Also, the issue is not that trivial. Just to give you one example:

struct v4l2_pix_format {
        __u32                   width;
       	__u32                   height;
        __u32                   pixelformat;
        enum v4l2_field         field;
        __u32                   bytesperline;   /* for padding, zero if unused */
        __u32                   sizeimage;
        enum v4l2_colorspace    colorspace;
       	__u32                   priv;           /* private data, depends on pixelformat */
};


This struct has 2 enums, and it is used by a couple structs, like this one:

struct v4l2_framebuffer {
        __u32                   capability;
       	__u32                   flags;
        void                    *base;
       	struct v4l2_pix_format  fmt;
};

This struct is used by a couple ioctls:

#define VIDIOC_G_FBUF            _IOR('V', 10, struct v4l2_framebuffer)
#define VIDIOC_S_FBUF            _IOW('V', 11, struct v4l2_framebuffer)

The better is to really replace "enum" by an integer (__u32?) at the structs,
but this will break existing apps.

One alternative would be to fork this header and add a compat layer
that would print a WARN_ONCE message, if ever reached, asking the user
to re-compile the application against the new header. 

We did that strategy in the past, appending _OLD to the legacy api ioctl's.

> 2) Add an version IOCTL that returns the current API level, so that
> the app can be written to support more than one API interface,
> depending on which kernel it is running on. The version IOCTL simply
> returns an u32 value. This is a consistent part of the user-kernel API
> that will never change.

There's one ioctl that already provides the API level, plus other info.
This ioctl doesn't contain any enum, so it is backward compatible.

> 3) Add "depreciated" compiler warnings to all the old API IOCTL calls,
> so app developers know they should be working to update their apps.

The issue here is with binaries compiled against the old headers. So, this
won't work.

> 4) After a few years, remove the old IOCTLs.
> 5) Use "uint32_t" and "uint64_t" types for all IOCTL calls, and not
> "unsigned int" or "unsigned long int".

No ioctls (well, except for 2 deprecated ones VIDIOC_G_JPEGCOMP/VIDIOC_S_JPEGCOMP)
are using __u8/__u32/__u64 for integers. The only issue there is with enum's.

> I.e. All structures passed in IOCTLs use fixed bit sized parameters
> for everything except of course pointers. Pointers depend on
> architecture.
> 6) Add a #if #endif around the old API, so a user compiling their own
> kernel can decide if the old API exists or not. User might want to do
> this for security reasons.

Add an #if block there will make the header very hard to deal with, as this
is already complex enough without it. The V4L2 API header has 2420 lines.
Such #if blocks will almost duplicate the header size.

I can see only two viable fixes for it:

1) add a typedef for the enum, using the sizeof(enum) in order to select the
size of the used integer.

Pros:
	- Patch is easy to write/easy to review;
	- Won't change the struct size, so applications compiled without
	  strong gcc optimization won't break;
Cons:
	- It will add a typedef, with is ugly;
	- struct size on 32 bits will be different thant he size on 64 bits
	  (not really an issue, as v4l2-compat32 will handle that;
	- v4l2-compat32 code may require changes.

2) just replace it by a 32 bits integer.

Pros:
	- no typedefs;
	- struct size won't change between 32/64 bits (except when they also
	  have pointers);
Cons:
	- will break ABI. So, a compat code is required;
	- will require a "videodev2.h" fork for the legacy API with the enum's;
	- will require a compat code to convert from enum into integer and
	  vice-versa.

Comments/Votes?
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Thu, 12 Apr 2012 11:55:12 -0300, Mauro Carvalho Chehab

<mchehab@redhat.com> wrote:

> I can see only two viable fixes for it:

> 

> 1) add a typedef for the enum, using the sizeof(enum) in order to select

> the

> size of the used integer.

> 

> Pros:

> 	- Patch is easy to write/easy to review;

> 	- Won't change the struct size, so applications compiled without

> 	  strong gcc optimization won't break;

> Cons:

> 	- It will add a typedef, with is ugly;

> 	- struct size on 32 bits will be different thant he size on 64 bits

> 	  (not really an issue, as v4l2-compat32 will handle that;



On which platforms do enums occupy 64-bits? Alpha? More to the point, on

which platform is enum not the same size as unsigned?



At least on x86-64, enum is 32-bits and so is unsigned.



> 	- v4l2-compat32 code may require changes.

> 

> 2) just replace it by a 32 bits integer.

> 

> Pros:

> 	- no typedefs;

> 	- struct size won't change between 32/64 bits (except when they also

> 	  have pointers);

> Cons:

> 	- will break ABI. So, a compat code is required;

> 	- will require a "videodev2.h" fork for the legacy API with the enum's;

> 	- will require a compat code to convert from enum into integer and

> 	  vice-versa.

> 

> Comments/Votes?
  

On 12 April 2012 15:55, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Em 12-04-2012 05:04, James Courtier-Dutton escreveu:
>> 6) Add a #if #endif around the old API, so a user compiling their own
>> kernel can decide if the old API exists or not. User might want to do
>> this for security reasons.
>
> Add an #if block there will make the header very hard to deal with, as this
> is already complex enough without it. The V4L2 API header has 2420 lines.
> Such #if blocks will almost duplicate the header size.
>

But it will work.
If you change the kernel-user API, this is a necessary evil you really
just have to do.

For ALSA, we had a #define ALSA_API_5 and #define ALSA_API_9.
The application writer defined one of these before including the
header file, and this specified which API version the application
writer used. This handles the use from user space.

After about 2 years, you can remove the old API version, and the
header file is then cleaned up.

You need to think about it as an API change.
So, you are really going from V4L2 to V4L3.

The kernel side of things is a bit messier.
You have to use different IOCTLs for the old API than the new API for
every IOCTL that has a changed parameter passed to it.
We managed to avoid this particular nasty, because in ALSA we had
libasound. So, we implemented all the nasty stuff in libasound,
letting the kernel only have to implement one API, either the new or
the old. So long as you installed a new libasound, the old application
would stay working.

I don't think you have something like libasound for v4l2 that every
application is using, so I would probably go with implementing V4L3.
I.e. A brand new API for Video in Linux.
You could say the driver for moving from V4L2 to V4L3 would be
security due to problems with emums.
Note: You can still use enums in the header file, but just pass their
value over the api as an int and not an emun type.
See the /linux/include/sound/asound.h for an example.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html