Accessing pointers inside struct passed as argument to ioctl calls
Hi all, I have a doubt about using pointers inside structs that are passed (as pointers) to ioctl argument. Since pointers passed from userspace can't be trusted, I need to copy they to kernel before accessing they. In this case I have a pointer inside a struct that is passed to the ioctl call also as pointer. So I need to copy the whole struct to kernel space and only then dereference the pointer. If this is true, two copy_from_user is needed right? Suppose I have a struct like this struct myioctl_arg { char *tx_buf; int tx_siz; } and this ioctl definition #define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg *); At userspace program I do something like this: struct myioctl_arg arg = { .tx_buf = somebuf, .tx_siz = 32 } ioctl(fd, MYIOCTL_TX, &arg); And in my ioclt implementation static ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { struct myioctl_arg karg; char kbuf[100]; ... case MYIOCTL_TX: copy_from_user(&karg, arg, sizeof(karg)); copy_from_user(kbuf, karg.tx_buf, karg.tx_siz); // now kbuf has the same contents of somebuf in userspace ... } I'm in doubt if this is the right way to access the userspace tx_buf. Since the .tx_buf from arg is a userspace pointer, accessible from userspace pointer arg, I can't type arg->tx_buf directly, doing this is dereferencing a userspace pointer inside kernel. So I need to copy the whole struct and dereference from the kernel space copy of it. So this two calls of copy_from_user() is the right way to do it? They are needed at all or is there better way of doing it? Cheers, -- "Do or do not. There is no try" Yoda Master
You are right, and thank you for bringing this to the mailing list to be sure about it. There are several catastrophic vulnerabilities I can see waiting to happen. First, you should be sure that the pointer that they passed in is checked, as in the pointer to the buffer should only reside in the mapped memory for their process. Second, you trust the size that they pass in with tx_size. You should take the minimum of tx_siz and size of your kbuf (which is a static 100). Be careful that when you record a static value to stub out this size, a number is implicitly by default a signed word size int to the compiler. So a #define would be signed, but if you change the type of int tx_siz, be careful. Also, likely you don't want negative buffer sizes. size_t or uint16_t should suit your purposes. On Mon, Sep 28, 2015 at 12:51 PM, Daniel. <danielhilst@gmail.com> wrote:
Hi all, I have a doubt about using pointers inside structs that are passed (as pointers) to ioctl argument. Since pointers passed from userspace can't be trusted, I need to copy they to kernel before accessing they. In this case I have a pointer inside a struct that is passed to the ioctl call also as pointer. So I need to copy the whole struct to kernel space and only then dereference the pointer. If this is true, two copy_from_user is needed right?
Suppose I have a struct like this
struct myioctl_arg { char *tx_buf; int tx_siz; }
and this ioctl definition
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg *);
At userspace program I do something like this:
struct myioctl_arg arg = { .tx_buf = somebuf, .tx_siz = 32 } ioctl(fd, MYIOCTL_TX, &arg);
And in my ioclt implementation static ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { struct myioctl_arg karg; char kbuf[100]; ... case MYIOCTL_TX: copy_from_user(&karg, arg, sizeof(karg)); copy_from_user(kbuf, karg.tx_buf, karg.tx_siz); // now kbuf has the same contents of somebuf in userspace ... }
I'm in doubt if this is the right way to access the userspace tx_buf. Since the .tx_buf from arg is a userspace pointer, accessible from userspace pointer arg, I can't type arg->tx_buf directly, doing this is dereferencing a userspace pointer inside kernel. So I need to copy the whole struct and dereference from the kernel space copy of it. So this two calls of copy_from_user() is the right way to do it? They are needed at all or is there better way of doing it?
Cheers, -- "Do or do not. There is no try" Yoda Master
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Okay, thank you for the tips Kenneth! This is not real code but the case is, so I will do that checks that you pointed out!! Thanks again! Best regards, - dhs 2015-09-28 14:09 GMT-03:00 Kenneth Adam Miller <kennethadammiller@gmail.com> :
You are right, and thank you for bringing this to the mailing list to be sure about it.
There are several catastrophic vulnerabilities I can see waiting to happen.
First, you should be sure that the pointer that they passed in is checked, as in the pointer to the buffer should only reside in the mapped memory for their process.
Second, you trust the size that they pass in with tx_size. You should take the minimum of tx_siz and size of your kbuf (which is a static 100). Be careful that when you record a static value to stub out this size, a number is implicitly by default a signed word size int to the compiler. So a #define would be signed, but if you change the type of int tx_siz, be careful. Also, likely you don't want negative buffer sizes. size_t or uint16_t should suit your purposes.
On Mon, Sep 28, 2015 at 12:51 PM, Daniel. <danielhilst@gmail.com> wrote:
Hi all, I have a doubt about using pointers inside structs that are passed (as pointers) to ioctl argument. Since pointers passed from userspace can't be trusted, I need to copy they to kernel before accessing they. In this case I have a pointer inside a struct that is passed to the ioctl call also as pointer. So I need to copy the whole struct to kernel space and only then dereference the pointer. If this is true, two copy_from_user is needed right?
Suppose I have a struct like this
struct myioctl_arg { char *tx_buf; int tx_siz; }
and this ioctl definition
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg *);
At userspace program I do something like this:
struct myioctl_arg arg = { .tx_buf = somebuf, .tx_siz = 32 } ioctl(fd, MYIOCTL_TX, &arg);
And in my ioclt implementation static ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { struct myioctl_arg karg; char kbuf[100]; ... case MYIOCTL_TX: copy_from_user(&karg, arg, sizeof(karg)); copy_from_user(kbuf, karg.tx_buf, karg.tx_siz); // now kbuf has the same contents of somebuf in userspace ... }
I'm in doubt if this is the right way to access the userspace tx_buf. Since the .tx_buf from arg is a userspace pointer, accessible from userspace pointer arg, I can't type arg->tx_buf directly, doing this is dereferencing a userspace pointer inside kernel. So I need to copy the whole struct and dereference from the kernel space copy of it. So this two calls of copy_from_user() is the right way to do it? They are needed at all or is there better way of doing it?
Cheers, -- "Do or do not. There is no try" Yoda Master
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
-- *"Do or do not. There is no try"* *Yoda Master*
Hi, Le lundi 28 septembre 2015 à 13:51 -0300, Daniel. a écrit :
Hi all, I have a doubt about using pointers inside structs that are passed (as pointers) to ioctl argument. Since pointers passed from userspace can't be trusted, I need to copy they to kernel before accessing they. In this case I have a pointer inside a struct that is passed to the ioctl call also as pointer. So I need to copy the whole struct to kernel space and only then dereference the pointer. If this is true, two copy_from_user is needed right?
Suppose I have a struct like this
struct myioctl_arg { char *tx_buf;
__u8 __user *tx_buf;
int tx_siz;
__u32 tx_siz;
}
and this ioctl definition
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg *);
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg);
At userspace program I do something like this:
struct myioctl_arg arg = { .tx_buf = somebuf, .tx_siz = 32 } ioctl(fd, MYIOCTL_TX, &arg);
And in my ioclt implementation static ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { struct myioctl_arg karg; char kbuf[100]; ... case MYIOCTL_TX: copy_from_user(&karg, arg, sizeof(karg));
if (copy_from_user(&karg, (const void __user *)arg, sizeof(karg)) return -EFAULT; if (!karg.tx_siz) return -EINVAL; if (karg.tx_siz > sizeof(kbuf)) return -EINVAL;
copy_from_user(kbuf, karg.tx_buf, karg.tx_siz);
if (copy_from_user(kbuf, karg.tx_buf, karg.tx_siz)) return -EFAULT;
// now kbuf has the same contents of somebuf in userspace ... }
I'm in doubt if this is the right way to access the userspace tx_buf. Since the .tx_buf from arg is a userspace pointer, accessible from userspace pointer arg, I can't type arg->tx_buf directly, doing this is dereferencing a userspace pointer inside kernel. So I need to copy the whole struct and dereference from the kernel space copy of it. So this two calls of copy_from_user() is the right way to do it? They are needed at all or is there better way of doing it?
They are needed. Add __user annotation, install sparse and build with make C=1. For the second one, if the tx_buf is large enough, you may be well using something like get_user_page() to pin userspace memory inside the kernel memory so that you don't have to copy the memory to access it. Regards. -- Yann Droneaud OPTEYA
Hi Yann, thank you, as I said this isn't real code, I just use to show my case. Anyway I will take the considerations in account. Thank you so much! And this get_user_page is new to me, thanks for pointing me out, I will read about it. The real thing is a driver to nrf24l01+ driver from Nordic. I may use this non copying aproach to exchange lot of frames without copying. This would improve driver's performance. :) Best regards! -dhs Em 28/09/2015 17:08, "Yann Droneaud" <ydroneaud@opteya.com> escreveu:
Hi,
Le lundi 28 septembre 2015 à 13:51 -0300, Daniel. a écrit :
Hi all, I have a doubt about using pointers inside structs that are passed (as pointers) to ioctl argument. Since pointers passed from userspace can't be trusted, I need to copy they to kernel before accessing they. In this case I have a pointer inside a struct that is passed to the ioctl call also as pointer. So I need to copy the whole struct to kernel space and only then dereference the pointer. If this is true, two copy_from_user is needed right?
Suppose I have a struct like this
struct myioctl_arg { char *tx_buf;
__u8 __user *tx_buf;
int tx_siz;
__u32 tx_siz;
}
and this ioctl definition
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg *);
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg);
At userspace program I do something like this:
struct myioctl_arg arg = { .tx_buf = somebuf, .tx_siz = 32 } ioctl(fd, MYIOCTL_TX, &arg);
And in my ioclt implementation static ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { struct myioctl_arg karg; char kbuf[100]; ... case MYIOCTL_TX: copy_from_user(&karg, arg, sizeof(karg));
if (copy_from_user(&karg, (const void __user *)arg, sizeof(karg)) return -EFAULT;
if (!karg.tx_siz) return -EINVAL;
if (karg.tx_siz > sizeof(kbuf)) return -EINVAL;
copy_from_user(kbuf, karg.tx_buf, karg.tx_siz);
if (copy_from_user(kbuf, karg.tx_buf, karg.tx_siz)) return -EFAULT;
// now kbuf has the same contents of somebuf in userspace ... }
I'm in doubt if this is the right way to access the userspace tx_buf. Since the .tx_buf from arg is a userspace pointer, accessible from userspace pointer arg, I can't type arg->tx_buf directly, doing this is dereferencing a userspace pointer inside kernel. So I need to copy the whole struct and dereference from the kernel space copy of it. So this two calls of copy_from_user() is the right way to do it? They are needed at all or is there better way of doing it?
They are needed. Add __user annotation, install sparse and build with make C=1.
For the second one, if the tx_buf is large enough, you may be well using something like get_user_page() to pin userspace memory inside the kernel memory so that you don't have to copy the memory to access it.
Regards.
-- Yann Droneaud OPTEYA
I'm pretty sure that exchanging ownership of memory pages between the kernel and userland is a really huge no-go for security as well. If you do that, you've implicitly given the user control of the memory map table contents, so you have to think like a malicious abuser of your api would. Copy from user exists for a reason, and I'm pretty sure that every budding kernel developer has suggested what you have because we want performance. But keep in mind you can only be as performant as reasonable without sacrificing your user's data on their behalf. On Mon, Sep 28, 2015 at 6:59 PM, Daniel. <danielhilst@gmail.com> wrote:
Hi Yann, thank you, as I said this isn't real code, I just use to show my case. Anyway I will take the considerations in account. Thank you so much! And this get_user_page is new to me, thanks for pointing me out, I will read about it.
The real thing is a driver to nrf24l01+ driver from Nordic. I may use this non copying aproach to exchange lot of frames without copying. This would improve driver's performance. :)
Best regards! -dhs Em 28/09/2015 17:08, "Yann Droneaud" <ydroneaud@opteya.com> escreveu:
Hi,
Le lundi 28 septembre 2015 à 13:51 -0300, Daniel. a écrit :
Hi all, I have a doubt about using pointers inside structs that are passed (as pointers) to ioctl argument. Since pointers passed from userspace can't be trusted, I need to copy they to kernel before accessing they. In this case I have a pointer inside a struct that is passed to the ioctl call also as pointer. So I need to copy the whole struct to kernel space and only then dereference the pointer. If this is true, two copy_from_user is needed right?
Suppose I have a struct like this
struct myioctl_arg { char *tx_buf;
__u8 __user *tx_buf;
int tx_siz;
__u32 tx_siz;
}
and this ioctl definition
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg *);
#define MYIOCTL_TX _IOWR(MY_MAGIC, MY_BASE, struct myioctl_arg);
At userspace program I do something like this:
struct myioctl_arg arg = { .tx_buf = somebuf, .tx_siz = 32 } ioctl(fd, MYIOCTL_TX, &arg);
And in my ioclt implementation static ioctl(struct file *fp, unsigned int cmd, unsigned long arg) { struct myioctl_arg karg; char kbuf[100]; ... case MYIOCTL_TX: copy_from_user(&karg, arg, sizeof(karg));
if (copy_from_user(&karg, (const void __user *)arg, sizeof(karg)) return -EFAULT;
if (!karg.tx_siz) return -EINVAL;
if (karg.tx_siz > sizeof(kbuf)) return -EINVAL;
copy_from_user(kbuf, karg.tx_buf, karg.tx_siz);
if (copy_from_user(kbuf, karg.tx_buf, karg.tx_siz)) return -EFAULT;
// now kbuf has the same contents of somebuf in userspace ... }
I'm in doubt if this is the right way to access the userspace tx_buf. Since the .tx_buf from arg is a userspace pointer, accessible from userspace pointer arg, I can't type arg->tx_buf directly, doing this is dereferencing a userspace pointer inside kernel. So I need to copy the whole struct and dereference from the kernel space copy of it. So this two calls of copy_from_user() is the right way to do it? They are needed at all or is there better way of doing it?
They are needed. Add __user annotation, install sparse and build with make C=1.
For the second one, if the tx_buf is large enough, you may be well using something like get_user_page() to pin userspace memory inside the kernel memory so that you don't have to copy the memory to access it.
Regards.
-- Yann Droneaud OPTEYA
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On Mon, 28 Sep 2015 19:59:41 -0300, "Daniel." said:
The real thing is a driver to nrf24l01+ driver from Nordic. I may use this non copying aproach to exchange lot of frames without copying. This would improve driver's performance. :)
There is already well-developed zero-copy code for several network drivers in the kernel tree. Don't re-invent existing code that's already debugged.
participants (4)
-
Daniel. -
Kenneth Adam Miller -
Valdis.Kletnieks＠vt.edu -
Yann Droneaud