From owner-freebsd-hackers@FreeBSD.ORG Mon Jan 21 03:15:44 2013 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 40CF68ED for ; Mon, 21 Jan 2013 03:15:44 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-qa0-f42.google.com (mail-qa0-f42.google.com [209.85.216.42]) by mx1.freebsd.org (Postfix) with ESMTP id 07A9A636 for ; Mon, 21 Jan 2013 03:15:43 +0000 (UTC) Received: by mail-qa0-f42.google.com with SMTP id hg5so6447676qab.8 for ; Sun, 20 Jan 2013 19:15:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=xrThzg3kKJNNeOb4/iV9ZnloALJ/aFzOgvK1GqsnLtI=; b=qD4Y4NO0HtGTjSxUEYJzmQlzP76RN2pvRlYG1RuBlGZtt/wN3dtKl5yIz5fdzs/TTD aw2oPR4ysos3xfMVv3hGNnd3FlCglQnE/GZklk02KU9psZa54FY9rWmhuhidmhFFubtq LBg+MWVGRBW909s14BlAsqu499fC2qvdDK/LfaYPR+EZWa1LIehJn0tIyxHT/gcckHxn 3ymUtLLvzlvjZBmee31RsyGENGrO1HJf1xP5a48jlRnwSvp2O9iJ7Wqolm3vjppc0Crx Bue0cXKpHa1FLesEqYTxXpSGNzpVDXi4Dj9irILb/3+/5D9bJUiXyuqad5b31c2DOExE LqBg== MIME-Version: 1.0 X-Received: by 10.229.203.28 with SMTP id fg28mr412857qcb.103.1358738137438; Sun, 20 Jan 2013 19:15:37 -0800 (PST) Sender: mdf356@gmail.com Received: by 10.229.156.18 with HTTP; Sun, 20 Jan 2013 19:15:37 -0800 (PST) In-Reply-To: <50FCACEC.8000100@rawbw.com> References: <50FC7767.4050207@rawbw.com> <50FCACEC.8000100@rawbw.com> Date: Sun, 20 Jan 2013 19:15:37 -0800 X-Google-Sender-Auth: W8LwcusvKTK56ivMHGvol2kWUa4 Message-ID: Subject: Re: How to validate the variable size memory block in ioctl handler? From: mdf@FreeBSD.org To: Yuri Content-Type: text/plain; charset=ISO-8859-1 Cc: hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Jan 2013 03:15:44 -0000 On Sun, Jan 20, 2013 at 6:50 PM, Yuri wrote: > On 01/20/2013 16:59, mdf@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 > 0xffffffff) { > 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 0x80000000 /* copy in parameters */ > #define IOC_INOUT (IOC_IN|IOC_OUT) > #define IOC_DIRMASK (IOC_VOID|IOC_OUT|IOC_IN) > +#define IOC_VARIABLE IOCPARM_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