netdev - Re: [Patch net-next] net_sched: fix a potential out-of-bound access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpWHPPZdRirBSYG89SjNfYyr4GVC9dsTM+jJYZ-+daNMww@mail.gmail.com>
Date:   Thu, 9 Aug 2018 14:43:12 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [Patch net-next] net_sched: fix a potential out-of-bound access

On Thu, Aug 9, 2018 at 12:32 AM Vlad Buslov <vladbu@...lanox.com> wrote:
>
> Before version V5 of my action API patchset this functionality was
> implemented in exactly the same way as in your patch. Unfortunately, it
> has a double-free bug. The problem is that if you have multiple
> actions(N) being deleted, and deleted succeeded for first K actions,
> this implementation will try to delete all N actions second time
> (including first K actions that were already deleted). That is why I
> added 'acts_deleted' variable that tracks actual amount of actions that
> were deleted successfully, and only delete last N-K actions in case of
> error.

Interesting, I didn't notice you call it for tcf_del_notify()'s failure too.

But this is easy to resolve, we can just set succeeded ones to NULL
and teach tcf_action_put_many() to scan the whole array but
skip NULL's.


>
> In order to fix that issue I did following code changes in V5:
> - Added 'acts_deleted' variable to delete only actions [K, N) in case of
> error.
> - Extended 'actions' array size by one to ensure that it always ends
> with NULL pointer.

Oh, I see, this is not how we use C, you can at least rollback
by passing acts_deleted as a parameter as the start of the array.
You picked the most confusing way to handle it.

I will send an updated patch.