Hello!
The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory
Allocator.
Syzkaller uses several methods [2] to limit memory consumption of the userspace
processes calling the syscalls for testing the kernel:
- setrlimit(),
- cgroups,
- various sysctl.
But these methods don't work for ION Memory Allocator, so any userspace process
that has access to /dev/ion can bring the system to the out-of-memory state.
An example of a program doing that:
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <linux/types.h>
#include <sys/ioctl.h>
#define ION_IOC_MAGIC 'I'
#define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \
struct ion_allocation_data)
struct ion_allocation_data {
__u64 len;
__u32 heap_id_mask;
__u32 flags;
__u32 fd;
__u32 unused;
};
int main(void)
{
unsigned long i = 0;
int fd = -1;
struct ion_allocation_data data = {
.len = 0x13f65d8c,
.heap_id_mask = 1,
.flags = 0,
.fd = -1,
.unused = 0
};
fd = open("/dev/ion", 0);
if (fd == -1) {
perror("[-] open /dev/ion");
return 1;
}
while (1) {
printf("iter %lu\n", i);
ioctl(fd, ION_IOC_ALLOC, &data);
i++;
}
return 0;
}
I looked through the code of ion_alloc() and didn't find any limit checks.
Is it currently possible to limit ION kernel allocations for some process?
If not, is it a right idea to do that?
Thanks!
Best regards,
Alexander
[1]: https://github.com/google/syzkaller
[2]: https://github.com/google/syzkaller/blob/master/executor/common_linux.h
On Wed, Jul 24, 2019 at 12:36 PM Laura Abbott <[email protected]> wrote:
>
> On 7/17/19 12:31 PM, Alexander Popov wrote:
> > Hello!
> >
> > The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory
> > Allocator.
> >
> > Syzkaller uses several methods [2] to limit memory consumption of the userspace
> > processes calling the syscalls for testing the kernel:
> > - setrlimit(),
> > - cgroups,
> > - various sysctl.
> > But these methods don't work for ION Memory Allocator, so any userspace process
> > that has access to /dev/ion can bring the system to the out-of-memory state.
> >
> > An example of a program doing that:
> >
> >
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > #include <stdio.h>
> > #include <linux/types.h>
> > #include <sys/ioctl.h>
> >
> > #define ION_IOC_MAGIC 'I'
> > #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \
> > struct ion_allocation_data)
> >
> > struct ion_allocation_data {
> > __u64 len;
> > __u32 heap_id_mask;
> > __u32 flags;
> > __u32 fd;
> > __u32 unused;
> > };
> >
> > int main(void)
> > {
> > unsigned long i = 0;
> > int fd = -1;
> > struct ion_allocation_data data = {
> > .len = 0x13f65d8c,
> > .heap_id_mask = 1,
> > .flags = 0,
> > .fd = -1,
> > .unused = 0
> > };
> >
> > fd = open("/dev/ion", 0);
> > if (fd == -1) {
> > perror("[-] open /dev/ion");
> > return 1;
> > }
> >
> > while (1) {
> > printf("iter %lu\n", i);
> > ioctl(fd, ION_IOC_ALLOC, &data);
> > i++;
> > }
> >
> > return 0;
> > }
> >
> >
> > I looked through the code of ion_alloc() and didn't find any limit checks.
> > Is it currently possible to limit ION kernel allocations for some process?
> >
> > If not, is it a right idea to do that?
> > Thanks!
> >
>
> Yes, I do think that's the right approach. We're working on moving Ion
> out of staging and this is something I mentioned to John Stultz. I don't
> think we've thought too hard about how to do the actual limiting so
> suggestions are welcome.
In part the dmabuf heaps allow for separate heap devices, so we can
have finer grained permissions to the specific heaps. But that
doesn't provide any controls on how much memory one process could
allocate using the device if it has permission.
I suspect the same issue is present with any of the dmabuf exporters
(gpu/display drivers, etc), so this is less of an ION/dmabuf heap
issue and more of a dmabuf core accounting issue.
Another practical complication is that with Android these days, I
believe the gralloc code lives in the HIDL-ized
[email protected] HAL, which does the
buffer allocations on behalf of requests sent over the binder IPC
interface. So with all dma-buf allocations effectively going through
that single process, I'm not sure we would want to put per-process
limits on the allocator. Instead, I suspect we'd want the memory
covered by the dmabuf to be accounted against processes that have the
dmabuf fd still open?
I know Android has some logic with their memtrack HAL to I believe try
to do accounting of gpu memory against various processes, but I've not
looked at that in detail recently.
Todd/Joel: Any input here?
thanks
-john
On Wed, Jul 24, 2019 at 1:18 PM John Stultz <[email protected]> wrote:
>
> On Wed, Jul 24, 2019 at 12:36 PM Laura Abbott <[email protected]> wrote:
> >
> > On 7/17/19 12:31 PM, Alexander Popov wrote:
> > > Hello!
> > >
> > > The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory
> > > Allocator.
> > >
> > > Syzkaller uses several methods [2] to limit memory consumption of the userspace
> > > processes calling the syscalls for testing the kernel:
> > > - setrlimit(),
> > > - cgroups,
> > > - various sysctl.
> > > But these methods don't work for ION Memory Allocator, so any userspace process
> > > that has access to /dev/ion can bring the system to the out-of-memory state.
> > >
> > > An example of a program doing that:
> > >
> > >
> > > #include <sys/types.h>
> > > #include <sys/stat.h>
> > > #include <fcntl.h>
> > > #include <stdio.h>
> > > #include <linux/types.h>
> > > #include <sys/ioctl.h>
> > >
> > > #define ION_IOC_MAGIC 'I'
> > > #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \
> > > struct ion_allocation_data)
> > >
> > > struct ion_allocation_data {
> > > __u64 len;
> > > __u32 heap_id_mask;
> > > __u32 flags;
> > > __u32 fd;
> > > __u32 unused;
> > > };
> > >
> > > int main(void)
> > > {
> > > unsigned long i = 0;
> > > int fd = -1;
> > > struct ion_allocation_data data = {
> > > .len = 0x13f65d8c,
> > > .heap_id_mask = 1,
> > > .flags = 0,
> > > .fd = -1,
> > > .unused = 0
> > > };
> > >
> > > fd = open("/dev/ion", 0);
> > > if (fd == -1) {
> > > perror("[-] open /dev/ion");
> > > return 1;
> > > }
> > >
> > > while (1) {
> > > printf("iter %lu\n", i);
> > > ioctl(fd, ION_IOC_ALLOC, &data);
> > > i++;
> > > }
> > >
> > > return 0;
> > > }
> > >
> > >
> > > I looked through the code of ion_alloc() and didn't find any limit checks.
> > > Is it currently possible to limit ION kernel allocations for some process?
> > >
> > > If not, is it a right idea to do that?
> > > Thanks!
> > >
> >
> > Yes, I do think that's the right approach. We're working on moving Ion
> > out of staging and this is something I mentioned to John Stultz. I don't
> > think we've thought too hard about how to do the actual limiting so
> > suggestions are welcome.
>
> In part the dmabuf heaps allow for separate heap devices, so we can
> have finer grained permissions to the specific heaps. But that
> doesn't provide any controls on how much memory one process could
> allocate using the device if it has permission.
>
> I suspect the same issue is present with any of the dmabuf exporters
> (gpu/display drivers, etc), so this is less of an ION/dmabuf heap
> issue and more of a dmabuf core accounting issue.
>
Also, do unmapped memfd buffers have similar accounting issues?
thanks
-john
On 7/17/19 12:31 PM, Alexander Popov wrote:
> Hello!
>
> The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory
> Allocator.
>
> Syzkaller uses several methods [2] to limit memory consumption of the userspace
> processes calling the syscalls for testing the kernel:
> - setrlimit(),
> - cgroups,
> - various sysctl.
> But these methods don't work for ION Memory Allocator, so any userspace process
> that has access to /dev/ion can bring the system to the out-of-memory state.
>
> An example of a program doing that:
>
>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <linux/types.h>
> #include <sys/ioctl.h>
>
> #define ION_IOC_MAGIC 'I'
> #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \
> struct ion_allocation_data)
>
> struct ion_allocation_data {
> __u64 len;
> __u32 heap_id_mask;
> __u32 flags;
> __u32 fd;
> __u32 unused;
> };
>
> int main(void)
> {
> unsigned long i = 0;
> int fd = -1;
> struct ion_allocation_data data = {
> .len = 0x13f65d8c,
> .heap_id_mask = 1,
> .flags = 0,
> .fd = -1,
> .unused = 0
> };
>
> fd = open("/dev/ion", 0);
> if (fd == -1) {
> perror("[-] open /dev/ion");
> return 1;
> }
>
> while (1) {
> printf("iter %lu\n", i);
> ioctl(fd, ION_IOC_ALLOC, &data);
> i++;
> }
>
> return 0;
> }
>
>
> I looked through the code of ion_alloc() and didn't find any limit checks.
> Is it currently possible to limit ION kernel allocations for some process?
>
> If not, is it a right idea to do that?
> Thanks!
>
Yes, I do think that's the right approach. We're working on moving Ion
out of staging and this is something I mentioned to John Stultz. I don't
think we've thought too hard about how to do the actual limiting so
suggestions are welcome.
Thanks,
Laura
> Best regards,
> Alexander
>
>
> [1]: https://github.com/google/syzkaller
> [2]: https://github.com/google/syzkaller/blob/master/executor/common_linux.h
>
On Wed, Jul 24, 2019 at 4:24 PM John Stultz <[email protected]> wrote:
>
> On Wed, Jul 24, 2019 at 1:18 PM John Stultz <[email protected]> wrote:
> >
> > On Wed, Jul 24, 2019 at 12:36 PM Laura Abbott <[email protected]> wrote:
> > >
> > > On 7/17/19 12:31 PM, Alexander Popov wrote:
> > > > Hello!
> > > >
> > > > The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION Memory
> > > > Allocator.
> > > >
> > > > Syzkaller uses several methods [2] to limit memory consumption of the userspace
> > > > processes calling the syscalls for testing the kernel:
> > > > - setrlimit(),
> > > > - cgroups,
> > > > - various sysctl.
> > > > But these methods don't work for ION Memory Allocator, so any userspace process
> > > > that has access to /dev/ion can bring the system to the out-of-memory state.
> > > >
> > > > An example of a program doing that:
> > > >
> > > >
> > > > #include <sys/types.h>
> > > > #include <sys/stat.h>
> > > > #include <fcntl.h>
> > > > #include <stdio.h>
> > > > #include <linux/types.h>
> > > > #include <sys/ioctl.h>
> > > >
> > > > #define ION_IOC_MAGIC 'I'
> > > > #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \
> > > > struct ion_allocation_data)
> > > >
> > > > struct ion_allocation_data {
> > > > __u64 len;
> > > > __u32 heap_id_mask;
> > > > __u32 flags;
> > > > __u32 fd;
> > > > __u32 unused;
> > > > };
> > > >
> > > > int main(void)
> > > > {
> > > > unsigned long i = 0;
> > > > int fd = -1;
> > > > struct ion_allocation_data data = {
> > > > .len = 0x13f65d8c,
> > > > .heap_id_mask = 1,
> > > > .flags = 0,
> > > > .fd = -1,
> > > > .unused = 0
> > > > };
> > > >
> > > > fd = open("/dev/ion", 0);
> > > > if (fd == -1) {
> > > > perror("[-] open /dev/ion");
> > > > return 1;
> > > > }
> > > >
> > > > while (1) {
> > > > printf("iter %lu\n", i);
> > > > ioctl(fd, ION_IOC_ALLOC, &data);
> > > > i++;
> > > > }
> > > >
> > > > return 0;
> > > > }
> > > >
> > > >
> > > > I looked through the code of ion_alloc() and didn't find any limit checks.
> > > > Is it currently possible to limit ION kernel allocations for some process?
> > > >
> > > > If not, is it a right idea to do that?
> > > > Thanks!
> > > >
> > >
> > > Yes, I do think that's the right approach. We're working on moving Ion
> > > out of staging and this is something I mentioned to John Stultz. I don't
> > > think we've thought too hard about how to do the actual limiting so
> > > suggestions are welcome.
> >
> > In part the dmabuf heaps allow for separate heap devices, so we can
> > have finer grained permissions to the specific heaps. But that
> > doesn't provide any controls on how much memory one process could
> > allocate using the device if it has permission.
> >
> > I suspect the same issue is present with any of the dmabuf exporters
> > (gpu/display drivers, etc), so this is less of an ION/dmabuf heap
> > issue and more of a dmabuf core accounting issue.
> >
>
> Also, do unmapped memfd buffers have similar accounting issues?
>
The syzcaller bot didn't complain about this for memfd yet, so I suspect not ;-)
With memfd since it uses shmem underneath, __vm_enough_memory() is
called during shmem_acct_block() which should take per-process memory
into account already and fail if there is not enough memory.
Should ION be doing something similar to fail if there's not enough memory?
thanks,
- Joel