Re: [PATCH] audit: fix memleak in auditd_send_unicast_skb.

[Paul Moore]

On Tue, Jul 18, 2017 at 8:10 PM, Shu Wang <shuwang@xxxxxxxxxx> wrote:
> ----- Original Message -----
>> From: "Paul Moore" <paul@xxxxxxxxxxxxxx>
>> To: shuwang@xxxxxxxxxx
>> Cc: "Eric Paris" <eparis@xxxxxxxxxx>, linux-audit@xxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, liwang@xxxxxxxxxx,
>> chuhu@xxxxxxxxxx
>> Sent: Tuesday, July 18, 2017 8:21:05 PM
>> Subject: Re: [PATCH] audit: fix memleak in auditd_send_unicast_skb.
>>
>> On Tue, Jul 18, 2017 at 2:37 AM,  <shuwang@xxxxxxxxxx> wrote:
>> > From: Shu Wang <shuwang@xxxxxxxxxx>
>> >
>> > Found this issue by kmemleak report, auditd_send_unicast_skb
>> > did not free skb if rcu_dereference(auditd_conn) returns null.
>> >
>> > unreferenced object 0xffff88082568ce00 (size 256):
>> > comm "auditd", pid 1119, jiffies 4294708499
>> > backtrace:
>> > [<ffffffff8176166a>] kmemleak_alloc+0x4a/0xa0
>> > [<ffffffff8121820c>] kmem_cache_alloc_node+0xcc/0x210
>> > [<ffffffff8161b99d>] __alloc_skb+0x5d/0x290
>> > [<ffffffff8113c614>] audit_make_reply+0x54/0xd0
>> > [<ffffffff8113dfa7>] audit_receive_msg+0x967/0xd70
>> > ----------------
>> > (gdb) list *audit_receive_msg+0x967
>> > 0xffffffff8113dff7 is in audit_receive_msg (kernel/audit.c:1133).
>> > 1132    skb = audit_make_reply(0, AUDIT_REPLACE, 0,
>> >                                 0, &pvnr, sizeof(pvnr));
>> > ---------------
>> > [<ffffffff8113e402>] audit_receive+0x52/0xa0
>> > [<ffffffff8166c561>] netlink_unicast+0x181/0x240
>> > [<ffffffff8166c8e2>] netlink_sendmsg+0x2c2/0x3b0
>> > [<ffffffff816112e8>] sock_sendmsg+0x38/0x50
>> > [<ffffffff816117a2>] SYSC_sendto+0x102/0x190
>> > [<ffffffff81612f4e>] SyS_sendto+0xe/0x10
>> > [<ffffffff8176d337>] entry_SYSCALL_64_fastpath+0x1a/0xa5
>> > [<ffffffffffffffff>] 0xffffffffffffffff
>> >
>> > Signed-off-by: Shu Wang <shuwang@xxxxxxxxxx>
>> > ---
>> >  kernel/audit.c | 1 +
>> >  1 file changed, 1 insertion(+)
>>
>> Hello and thank you for the problem report, it is appreciated.  This
>> was also reported by Masami Ichikawa who provided a patch with the
>> correct fix (your patch does not catch any error conditions from
>> netlink_unicast()).
>
> netlink_unicast has it's error handling, and is responsible for
> releasing skb. so the Masami Ichikawa's fix may cause double
> free problems.

Yes, my apologies, you are correct.  I've backed out the other patch
and applied your fix to audit/stable-4.13.

-- 
paul moore
www.paul-moore.com