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.
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
|
