Messages in this thread | | | Subject | Re: [PATCH next] audit: fix potential double free on error path from fsnotify_add_inode_mark | From | cuigaosheng <> | Date | Tue, 23 Aug 2022 10:06:24 +0800 |
| |
在 2022/8/23 6:49, Paul Moore 写道: > On Mon, Aug 22, 2022 at 1:59 PM Jan Kara <jack@suse.cz> wrote: >> On Mon 22-08-22 13:39:23, Paul Moore wrote: >>> On Mon, Aug 22, 2022 at 11:20 AM Jan Kara <jack@suse.cz> wrote: >>>> On Mon 22-08-22 10:34:15, Paul Moore wrote: >>>>> On Mon, Aug 22, 2022 at 4:50 AM Jan Kara <jack@suse.cz> wrote: >>>>>> On Mon 22-08-22 10:29:05, Gaosheng Cui wrote: >>>>>>> Audit_alloc_mark() assign pathname to audit_mark->path, on error path >>>>>>> from fsnotify_add_inode_mark(), fsnotify_put_mark will free memory >>>>>>> of audit_mark->path, but the caller of audit_alloc_mark will free >>>>>>> the pathname again, so there will be double free problem. >>>>>>> >>>>>>> Fix this by resetting audit_mark->path to NULL pointer on error path >>>>>>> from fsnotify_add_inode_mark(). >>>>>>> >>>>>>> Fixes: 7b1293234084d ("fsnotify: Add group pointer in fsnotify_init_mark()") >>>>>>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >>>>>> Good spotting! The patch looks good to me. Feel free to add: >>>>>> >>>>>> Reviewed-by: Jan Kara <jack@suse.cz> >>>>>> >>>>>>> --- >>>>>>> kernel/audit_fsnotify.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c >>>>>>> index 6432a37ac1c9..c565fbf66ac8 100644 >>>>>>> --- a/kernel/audit_fsnotify.c >>>>>>> +++ b/kernel/audit_fsnotify.c >>>>>>> @@ -102,6 +102,7 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa >>>>>>> >>>>>>> ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, 0); >>>>>>> if (ret < 0) { >>>>>>> + audit_mark->path = NULL; >>>>>>> fsnotify_put_mark(&audit_mark->mark); >>>>> As I'm tracing the code path from audit through fsnotify, and back >>>>> into audit, I'm wondering if we still have a problem. When >>>>> fsnotify_add_inode_mark() fails it will end up freeing not just >>>>> audit_mark->path, but audit_mark itself via audit_fsnotify_mark_free() >>>>> (via a call into fsnotify_put_mark()), yes? >>>> I don't think so. fsnotify_add_mark_locked() will call fsnotify_put_mark() >>>> but that is just a counter part to fsnotify_get_mark() a few lines above. >>>> The caller of fsnotify_add_inode_mark() still holds its own mark reference >>>> which prevents mark from being freed. >>> Okay, that sounds reasonable, but I'm still looking for a code path >>> that only frees audit_mark:path and not the audit_mark itself. What >>> am I not seeing? >> The callers of audit_alloc_mark() call kfree(path) if audit_alloc_mark() >> returns error (which is a sensible thing because in some cases path indeed >> needs freeing). > Of course! Thanks for that, I think I got a bit of tunnel vision on > this for some reason. > > I'll merge this into audit/stable-6.0 now and once testing is complete > I'll send it up to Linus. Thanks everyone!
Thanks for taking the time to review this patch. Thanks everyone!
| |