• Vladimir Oltean's avatar
    net: mscc: ocelot: mark traps with a bool instead of keeping them in a list · e1846cff
    Vladimir Oltean authored
    Since the blamed commit, VCAP filters can appear on more than one list.
    If their action is "trap", they are chained on ocelot->traps via
    filter->trap_list. This is in addition to their normal placement on the
    VCAP block->rules list head.
    
    Therefore, when we free a VCAP filter, we must remove it from all lists
    it is a member of, including ocelot->traps.
    
    There are at least 2 bugs which are direct consequences of this design
    decision.
    
    First is the incorrect usage of list_empty(), meant to denote whether
    "filter" is chained into ocelot->traps via filter->trap_list.
    This does not do the correct thing, because list_empty() checks whether
    "head->next == head", but in our case, head->next == head->prev == NULL.
    So we dereference NULL pointers and die when we call list_del().
    
    Second is the fact that not all places that should remove the filter
    from ocelot->traps do so. One example is ocelot_vcap_block_remove_filter(),
    which is where we have the main kfree(filter). By keeping freed filters
    in ocelot->traps we end up in a use-after-free in
    felix_update_trapping_destinations().
    
    Attempting to fix all the buggy patterns is a whack-a-mole game which
    makes the driver unmaintainable. Actually this is what the previous
    patch version attempted to do:
    https://patchwork.kernel.org/project/netdevbpf/patch/20220503115728.834457-3-vladimir.oltean@nxp.com/
    
    but it introduced another set of bugs, because there are other places in
    which create VCAP filters, not just ocelot_vcap_filter_create():
    
    - ocelot_trap_add()
    - felix_tag_8021q_vlan_add_rx()
    - felix_tag_8021q_vlan_add_tx()
    
    Relying on the convention that all those code paths must call
    INIT_LIST_HEAD(&filter->trap_list) is not going to scale.
    
    So let's do what should have been done in the first place and keep a
    bool in struct ocelot_vcap_filter which denotes whether we are looking
    at a trapping rule or not. Iterating now happens over the main VCAP IS2
    block->rules. The advantage is that we no longer risk having stale
    references to a freed filter, since it is only present in that list.
    
    Fixes: e42bd4ed ("net: mscc: ocelot: keep traps in a list")
    Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
    Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
    e1846cff
felix.c 53 KB