Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get FAULT when fuzzing sys_ring_buf_ put and sys_ring_bug_get APIs #7638

Closed
stuartly opened this issue May 17, 2018 · 11 comments
Closed

get FAULT when fuzzing sys_ring_buf_ put and sys_ring_bug_get APIs #7638

stuartly opened this issue May 17, 2018 · 11 comments
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Security Review To be reviewed by a security expert

Comments

@stuartly
Copy link

I am trying to fuzzing the kernel APIs, and I wrote a simple application to call the APIs: sys_ring_buf_put and sys_ring_buf_get.

the code is as below

 while (1) {

                type = sys_rand32_get();
                value = sys_rand32_get();
                dsize = sys_rand32_get();
                printk("type:%d, value:%d, dsize: %d\n", type, value, dsize);

                /*testing APIs*/
                ret = sys_ring_buf_put(&ring_buf1, type, value,
                                       rb_data, dsize);
                if (ret == -EMSGSIZE) {
                        printk("ring buffer is full\n");
                        break;
                }
                printk("inserted %d chunks, %d remaining\n", dsize,
                            sys_ring_buf_space_get(&ring_buf1));
                dsize = (dsize + 1) % SIZE32_OF(rb_data);
                put_count++;
        }

        getsize = sys_rand32_get() - 1;
        ret = sys_ring_buf_get(&ring_buf1, &gettype, &getval, getdata, &getsize);
        if (ret != -EMSGSIZE) {
                printk("Allowed retreival with insufficient destination buffer space\n");
        }

        for (i = 0; i < put_count; i++) {
                getsize = SIZE32_OF(getdata);
                ret = sys_ring_buf_get(&ring_buf1, &gettype, &getval, getdata,
                                       &getsize);
                printk("got %u chunks of type %u and val %u, %u remaining\n",
                            getsize, gettype, getval,
                            sys_ring_buf_space_get(&ring_buf1));

        }

when I run the application, it result in USEAGE FAULT as following:

type:1453511663, value:-1841455610, dsize: -841455592
inserted -841455592 chunks, 6 remaining
type:158548800, value:1158548820, dsize: -2136418458
ring buffer is full
Allowed retreival with insufficient destination buffer space
got 6 chunks of type 54255 and val 6, 31 remaining
***** USAGE FAULT *****
  Executing thread ID (thread): 0x20000228
  Faulting instruction address:  0xf21
Fatal fault in essential thread! Spinning...

Should zephyr add some checks like type and bound check in the implementation of syscall API code ?

@andrewboie
Copy link
Contributor

the ring buffer APIs aren't system calls, they don't do privilege elevation.
In general to keep code as small as possible for tiny devices, checking like this isn't done in Zephyr.
We could maybe add some __ASSERT()s though.

@andrewboie
Copy link
Contributor

Correction, looking at the RB code again it does return error codes when problems are detected.

For your test case, I suggest using %u for the output as all the API arguments are treated as unsigned.

Not sure I understand this bit:

        getsize = sys_rand32_get() - 1;
        ret = sys_ring_buf_get(&ring_buf1, &gettype, &getval, getdata, &getsize);
        if (ret != -EMSGSIZE) {
                printk("Allowed retreival with insufficient destination buffer space\n");
        }

How do we know for sure getsize is larger than the destination space if it is random?

Anyway this all looks like a bug.

@andrewboie andrewboie added the bug The issue is a bug, or the PR is fixing a bug label May 17, 2018
@MaureenHelm MaureenHelm added the priority: medium Medium impact/importance bug label May 21, 2018
@andrewboie
Copy link
Contributor

@stuartly I'm having trouble getting your sample code fragment to compile.
Can you please send me the complete source to your test application?

@nashif nashif added the Security Review To be reviewed by a security expert label Jun 5, 2018
@nashif nashif assigned ManojSubbarao and unassigned andrewboie Jun 7, 2018
@nashif
Copy link
Member

nashif commented Jun 7, 2018

@ManojSubbarao please assign to your team

@nashif nashif added priority: low Low impact/importance bug and removed priority: medium Medium impact/importance bug labels Jun 7, 2018
@stuartly
Copy link
Author

stuartly commented Jun 7, 2018

@andrewboie Sorry to response late. The sample code is in the attachment.

main.txt

@ManojSubbarao
Copy link

@varun-sha , Please look into this issue.

@varun-sha
Copy link
Contributor

I tried it on qemu _x86, and got page fault with null ptr exception consistently.

Error:-
type:-2092918928, value:-2092917264, dsize: -2092916140
ring buffer is full
Allowed retreival with insufficient destination buffer space:0
got 6 chunks of type 56550 and val 88, 31 remaining
***** CPU Page Fault (error code 0x00000010)
Supervisor thread executed address 0x00000000
PDE: 0x025 Present, Read-only, User, Execute Enabled
PTE: 0x00 Non-present, Read-only, Supervisor, Execute Enabled
Current thread ID = 0x004011c0
eax: 0xfffffff5, ebx: 0x00000000, ecx: 0x00400000, edx: 0x0000001b
esi: 0x00000000, edi: 0x00000000, ebp: 0x00000000, esp: 0x004083c8
eflags: 0x00000202 cs: 0x0008
call trace:
eip: 0x00000000
NULL base ptr
Fatal fault in essential thread! Spinning...
Terminate emulator due to fatal kernel error

prj.conf is not shared with test code, so i used below one:-
CONFIG_IRQ_OFFLOAD=y
CONFIG_RING_BUFFER=y
CONFIG_CONSOLE_GETLINE=y
CONFIG_CONSOLE_PULL=y

looking into it, to find root cause.

@varun-sha
Copy link
Contributor

varun-sha commented Dec 24, 2018

@stuartly Hi, i have gone through your sample fuzzy app, during fuzzing its fine to pass random values, but you are initializing smaller buffer(u32_t getdata[6] in your sample) and passing random getsize value, getsize should be limited to max buffer in which you want to copy data from ring_buf_item_get

sometimes when you do ring_buf_item_get you are overwriting memory, which is wrong and causing panic.
ring_buf_item_get() requires buffer & size of buffer as parameter, you can't pass size as larger then allocated buffer size.
https://docs.zephyrproject.org/1.13.0/kernel/other/ring_buffers.html#retrieving-data

Just change your code to limit random value of getsize to max buffer you initialized, it will work fine with all fuzzing you are trying with type/value/size.
getsize = sys_rand32_get()%SIZE32_OF(getdata);

Please retest and close if it works fine at your end.

@stuartly
Copy link
Author

@varun-sha Thanks, it is fine.

@varun-sha
Copy link
Contributor

@varun-sha Thanks, it is fine.

@stuartly : can you close this issue then , thanks

@varun-sha
Copy link
Contributor

@varun-sha Thanks, it is fine.

@stuartly : can you close this issue then , thanks

@stuartly : please close this issue, since we are good with analysis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug Security Review To be reviewed by a security expert
Projects
None yet
Development

No branches or pull requests

6 participants