Re: How to validate the variable size memory block in ioctl handler?

[mdf]

On Sun, Jan 20, 2013 at 3:01 PM, Yuri y...@rawbw.com wrote:
 I am implementing an ioctl that reads/writes variable size structure.
 Allocated size is supplied by the caller in the structure itself.
 struct my_struct {
   int len; // allocated size
   other_struct s[1];
 };
 ioctl request id is defined as _IOWR('X', number, my_struct)

 How to validate from the ioctl function handler (for some device) that the
 whole (variable size) block of bytes is RW accessible in the process memory
 space?
 Should I call copyout/copyin for this, or there is some shorter way?
 EFAULT should be returned in case of validation failure.

 As I understand, macros like _IOR, _IOWR do validation based on the size of
 structure supplied to them. So that the handler procedures don't have to do
 that.
 I was expecting to find among them some macro that would work for such
 variable size structure, but it isn't there. (Not sure if this is possible
 language-wise).

You'll need to pass in more than the above, probably, as the kernel's
ioctl() function has copied in the specified number of bytes already.
I.e. the value passed to your ioctl handler is already in the kernel
space, and unless it's 4 bytes, was malloc(9)'d and copyin'd (if it's
an IN parameter).  The size used is the size passed to the _IOC()
macro.

To do what you want it sounds like you want your handler to take something like:

struct var_ioctl {
int len;
void *data;
};

Then then handler itself would have to use copyin/copyout to access
the data.  There's no simpler way.

Cheers,
matthew
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org

Re: How to validate the variable size memory block in ioctl handler?

[Yuri]

On 01/20/2013 16:59, m...@freebsd.org wrote:

To do what you want it sounds like you want your handler to take something like:

struct var_ioctl {
 int len;
 void *data;
};

Then then handler itself would have to use copyin/copyout to access
the data.  There's no simpler way.


I think I found the simpler way, see the draft patch below.
Generic macro _IOWRE will handle the case when the first integer in 
ioctl parameter holds the actual size of the structure.
This way of passing the variable array sizes is quite common in various 
APIs.

Other potential uses would also benefit from this.

Yuri


Index: sys/kern/sys_generic.c
===
--- sys/kern/sys_generic.c  (revision 245654)
+++ sys/kern/sys_generic.c  (working copy)
@@ -640,6 +640,7 @@
int arg, error;
u_int size;
caddr_t data;
+   int vsize;
 
 	if (uap-com  0x) {

printf(
@@ -654,6 +655,14 @@
 * copied to/from the user's address space.
 */
size = IOCPARM_LEN(com);
+   if (size == IOC_VARIABLE) {
+   /* first integer has the length of the memory */
+   error = copyin(uap-data, (caddr_t)vsize, sizeof(vsize));
+   if (error)
+   return (error);
+   size = (u_int)vsize;
+   }
if ((size  IOCPARM_MAX) ||
((com  (IOC_VOID  | IOC_IN | IOC_OUT)) == 0) ||
 #if defined(COMPAT_FREEBSD5) || defined(COMPAT_FREEBSD4) || defined(COMPAT_43)
Index: sys/sys/ioccom.h
===
--- sys/sys/ioccom.h(revision 245654)
+++ sys/sys/ioccom.h(working copy)
@@ -50,6 +50,7 @@
 #defineIOC_IN  0x8000  /* copy in parameters */
 #defineIOC_INOUT   (IOC_IN|IOC_OUT)
 #defineIOC_DIRMASK (IOC_VOID|IOC_OUT|IOC_IN)
+#defineIOC_VARIABLEIOCPARM_MASK/* parameters size in 
parameters */
 
 #define	_IOC(inout,group,num,len)	((unsigned long) \

((inout) | (((len)  IOCPARM_MASK)  16) | ((group)  8) | (num)))
@@ -59,6 +60,9 @@
 #define_IOW(g,n,t) _IOC(IOC_IN,(g), (n), sizeof(t))
 /* this should be _IORW, but stdio got there first */
 #define_IOWR(g,n,t)_IOC(IOC_INOUT, (g), (n), sizeof(t))
+#define_IORE(g,n)  _IOC(IOC_OUT,   (g), (n), IOC_VARIABLE)
+#define_IOWE(g,n)  _IOC(IOC_IN,(g), (n), IOC_VARIABLE)
+#define_IOWRE(g,n) _IOC(IOC_INOUT, (g), (n), IOC_VARIABLE)
 
 #ifdef _KERNEL




___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org

Re: How to validate the variable size memory block in ioctl handler?

[mdf]

On Sun, Jan 20, 2013 at 6:50 PM, Yuri y...@rawbw.com wrote:
 On 01/20/2013 16:59, m...@freebsd.org wrote:

 To do what you want it sounds like you want your handler to take something
 like:

 struct var_ioctl {
 int len;
 void *data;
 };

 Then then handler itself would have to use copyin/copyout to access
 the data.  There's no simpler way.


 I think I found the simpler way, see the draft patch below.
 Generic macro _IOWRE will handle the case when the first integer in ioctl
 parameter holds the actual size of the structure.
 This way of passing the variable array sizes is quite common in various
 APIs.
 Other potential uses would also benefit from this.

 Yuri


 Index: sys/kern/sys_generic.c
 ===
 --- sys/kern/sys_generic.c(revision 245654)
 +++ sys/kern/sys_generic.c(working copy)
 @@ -640,6 +640,7 @@
   int arg, error;
   u_int size;
   caddr_t data;
 + int vsize;

   if (uap-com  0x) {
   printf(
 @@ -654,6 +655,14 @@
* copied to/from the user's address space.
*/
   size = IOCPARM_LEN(com);
 + if (size == IOC_VARIABLE) {
 + /* first integer has the length of the memory */
 + error = copyin(uap-data, (caddr_t)vsize, sizeof(vsize));
 + if (error)
 + return (error);
 + size = (u_int)vsize;
 + }
   if ((size  IOCPARM_MAX) ||
   ((com  (IOC_VOID  | IOC_IN | IOC_OUT)) == 0) ||
  #if defined(COMPAT_FREEBSD5) || defined(COMPAT_FREEBSD4) ||
 defined(COMPAT_43)
 Index: sys/sys/ioccom.h
 ===
 --- sys/sys/ioccom.h  (revision 245654)
 +++ sys/sys/ioccom.h  (working copy)
 @@ -50,6 +50,7 @@
  #define  IOC_IN  0x8000  /* copy in parameters */
  #define  IOC_INOUT   (IOC_IN|IOC_OUT)
  #define  IOC_DIRMASK (IOC_VOID|IOC_OUT|IOC_IN)
 +#define  IOC_VARIABLEIOCPARM_MASK/* parameters size in 
 parameters */

  #define  _IOC(inout,group,num,len)   ((unsigned long) \
   ((inout) | (((len)  IOCPARM_MASK)  16) | ((group)  8) | (num)))
 @@ -59,6 +60,9 @@
  #define  _IOW(g,n,t) _IOC(IOC_IN,(g), (n), sizeof(t))
  /* this should be _IORW, but stdio got there first */
  #define  _IOWR(g,n,t)_IOC(IOC_INOUT, (g), (n), sizeof(t))
 +#define  _IORE(g,n)  _IOC(IOC_OUT,   (g), (n), IOC_VARIABLE)
 +#define  _IOWE(g,n)  _IOC(IOC_IN,(g), (n), IOC_VARIABLE)
 +#define  _IOWRE(g,n) _IOC(IOC_INOUT, (g), (n), IOC_VARIABLE)

  #ifdef _KERNEL


This would be fine for a local patch but it breaks existing (valid)
uses that have exactly 8191 bytes of data, so it wouldn't be suitable
for the main FreeBSD repository.  Also, in general one wants to have
limits on syscalls that can force a kernel malloc of any size, as it
leads to denial of service attacks or crashes by requesting the kernel
over-allocate memory.

Cheers,
matthew
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org

Re: How to validate the variable size memory block in ioctl handler?

[Yuri]

On 01/20/2013 19:15, m...@freebsd.org wrote:

This would be fine for a local patch but it breaks existing (valid)
uses that have exactly 8191 bytes of data, so it wouldn't be suitable
for the main FreeBSD repository.  Also, in general one wants to have
limits on syscalls that can force a kernel malloc of any size, as it
leads to denial of service attacks or crashes by requesting the kernel
over-allocate memory.


Both problems are easily fixable.
Current len range can be preserved by encoding this case into an 'inout' 
parameter of _IOC instead. IOC_VOID is only used when no IOC_IN/IOC_OUT 
is set, so all 3 bits would mean _IORWE.
And arbitrarily high parameter size can be explicitly limited in 
sys_generic.c to IOCPARM_MAX.


Yuri
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org