Recently I saw a post to the linux kernel mailing-list containing a simple fix for a use-after-free bug. The code in question originally read:
hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len); if (IS_ERR(hdr->pkcs7_msg)) { kfree(hdr); return PTR_ERR(hdr->pkcs7_msg); }
Here the bug is obvious once it has been pointed out:
- A structure is freed.
- But then it is dereferenced, to provide a return value.
This is the kind of bug that would probably have been obvious to me if I'd happened to read the code myself. However patch submitted so job done? I did have some free time so I figured I'd scan for similar bugs. Writing a trivial perl script to look for similar things didn't take too long, though it is a bit shoddy:
- Open each file.
- If we find a line containing "free(.*)" record the line and the thing that was freed.
- The next time we find a
return
look to see if the return value uses the thing that was free'd.- If so that's a possible bug. Report it.
Of course my code is nasty, but it looked like it immediately paid off. I found this snippet of code in linux-5.2.8/drivers/media/pci/tw68/tw68-video.c
:
if (hdl->error) { v4l2_ctrl_handler_free(hdl); return hdl->error; }
That looks promising:
- The structure
hdl
is freed, via a dedicated freeing-function. - But then we return the member
error
from it.
Chasing down the code I found that linux-5.2.8/drivers/media/v4l2-core/v4l2-ctrls.c
contains the code for the v4l2_ctrl_handler_free
call and while it doesn't actually free the structure - just some members - it does reset the contents of hdl->error
to zero.
Ahah! The code I've found looks for an error, and if it was found returns zero, meaning the error is lost. I can fix it, by changing to this:
if (hdl->error) { int err = hdl->error; v4l2_ctrl_handler_free(hdl); return err; }
I did that. Then looked more closely to see if I was missing something. The code I've found lives in the function tw68_video_init1
, that function is called only once, and the return value is ignored!
So, that's the story of how I scanned the Linux kernel for use-after-free bugs and contributed nothing to anybody.
Still fun though.
I'll go over my list more carefully later, but nothing else jumped out as being immediately bad.
There is a weird case I spotted in ./drivers/media/platform/s3c-camif/camif-capture.c
with a similar pattern. In that case the function involved is s3c_camif_create_subdev
which is invoked by ./drivers/media/platform/s3c-camif/camif-core.c
:
ret = s3c_camif_create_subdev(camif); if (ret < 0) goto err_sd;
So I suspect there is something odd there:
- If there's an error in
s3c_camif_create_subdev
- Then
handler->error
will be reset to zero. - Which means that
return handler->error
will return 0. - Which means that the
s3c_camif_create_subdev
call should have returned an error, but won't be recognized as having done so. - i.e. "0 < 0" is false.
- Then
Of course the error-value is only set if this code is hit:
hdl->buckets = kvmalloc_array(hdl->nr_of_buckets, sizeof(hdl->buckets[0]), GFP_KERNEL | __GFP_ZERO); hdl->error = hdl->buckets ? 0 : -ENOMEM;
Which means that the registration of the sub-device fails if there is no memory, and at that point what can you even do?
It's a bug, but it isn't a security bug.
Tags: kernel, lkml, security 2 comments