Re: [net-mellanox] question about potential null pointer dereference

[Gustavo A. R. Silva]

Quoting Ido Schimmel <idosch@xxxxxxxxxxxx>:

> On Wed, May 10, 2017 at 10:36:59AM -0500, Gustavo A. R. Silva wrote:
> > Hello everybody,
> >
> > While looking into Coverity ID 1350941 I ran into the following piece of
> > code at drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c:1483:
> >
> > 1483static void mlxsw_sp_fdb_notify_mac_process(struct mlxsw_sp *mlxsw_sp,
> > 1484                                            char *sfn_pl, int rec_index,
> > 1485                                            bool adding)
> > 1486{
> > 1487        struct mlxsw_sp_port *mlxsw_sp_port;
> > 1488        char mac[ETH_ALEN];
> > 1489        u8 local_port;
> > 1490        u16 vid, fid;
> > 1491        bool do_notification = true;
> > 1492        int err;
> > 1493
> > 1494        mlxsw_reg_sfn_mac_unpack(sfn_pl, rec_index, mac, &fid,
> > &local_port);
> > 1495        mlxsw_sp_port = mlxsw_sp->ports[local_port];
> > 1496        if (!mlxsw_sp_port) {
> > 1497                dev_err_ratelimited(mlxsw_sp->bus_info->dev, "Incorrect
> > local port in FDB notification\n");
> > 1498                goto just_remove;
> > 1499        }
> > 1500
> > 1501        if (mlxsw_sp_fid_is_vfid(fid)) {
> > 1502                struct mlxsw_sp_port *mlxsw_sp_vport;
> > 1503
> > 1504                mlxsw_sp_vport =
> > mlxsw_sp_port_vport_find_by_fid(mlxsw_sp_port,
> > 1505                                                                 fid);
> > 1506                if (!mlxsw_sp_vport) {
> > 1507                        netdev_err(mlxsw_sp_port->dev, "Failed to find a
> > matching vPort following FDB notification\n");
> > 1508                        goto just_remove;
> > 1509                }
> > 1510                vid = 0;
> > 1511                /* Override the physical port with the vPort. */
> > 1512                mlxsw_sp_port = mlxsw_sp_vport;
> > 1513        } else {
> > 1514                vid = fid;
> > 1515        }
> > 1516
> > 1517do_fdb_op:
> > 1518        err = mlxsw_sp_port_fdb_uc_op(mlxsw_sp, local_port, mac, fid,
> > 1519                                      adding, true);
> > 1520        if (err) {
> > 1521                if (net_ratelimit())
> > 1522                        netdev_err(mlxsw_sp_port->dev, "Failed to set
> > FDB entry\n");
> > 1523                return;
> > 1524        }
> > 1525
> > 1526        if (!do_notification)
> > 1527                return;
> > 1528        mlxsw_sp_fdb_call_notifiers(mlxsw_sp_port->learning_sync,
> > 1529                                    adding, mac, vid,  
> > mlxsw_sp_port->dev);
> > 1530        return;
> > 1531
> > 1532just_remove:
> > 1533        adding = false;
> > 1534        do_notification = false;
> > 1535        goto do_fdb_op;
> > 1536}
> >
> >
> > The issue here is that line 1496 implies that mlxsw_sp_port might be NULL.
> > If this is the case, the execution path jumps to line 1532 and then to line
> > 1517. All this could end up dereferencing a NULL pointer at line 1522.
> >
> > Is there any chance for mlxsw_sp_port to be NULL at line 1496 and, at the
> > same time, a NULL pointer dereference occurs at line 1522?
> >
> > I'm trying to figure out if this is a false positive or something that
> > actually needs to be fixed.
>
> In theory, yes, it can happen, but it didn't happen yet. I recently
> patched that and now it's in Jiri's queue. I guess he'll send it
> tomorrow.
>
> https://github.com/jpirko/linux_mlxsw/commit/4160fb9ad6eeacba7736cfdbf7f52248432c2e89

Great, glad to see it is fixed now.

> Thanks for looking into this!

Sure thing, thanks for clarifying. :)
--
Gustavo A. R. Silva