* [net-mellanox] question about potential null pointer dereference
@ 2017-05-10 15:36 Gustavo A. R. Silva
2017-05-10 15:51 ` Ido Schimmel
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-10 15:36 UTC (permalink / raw)
To: Jiri Pirko, Ido Schimmel; +Cc: netdev, linux-kernel
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.
I'd really appreciate any comment on this.
Thank you!
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [net-mellanox] question about potential null pointer dereference
2017-05-10 15:36 [net-mellanox] question about potential null pointer dereference Gustavo A. R. Silva
@ 2017-05-10 15:51 ` Ido Schimmel
2017-05-10 16:01 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Ido Schimmel @ 2017-05-10 15:51 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Jiri Pirko, netdev, linux-kernel
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
Thanks for looking into this!
>
> I'd really appreciate any comment on this.
> Thank you!
> --
> Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [net-mellanox] question about potential null pointer dereference
2017-05-10 15:51 ` Ido Schimmel
@ 2017-05-10 16:01 ` Gustavo A. R. Silva
0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-10 16:01 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Jiri Pirko, netdev, linux-kernel
Quoting Ido Schimmel <idosch@mellanox.com>:
> 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. :)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-05-10 16:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-10 15:36 [net-mellanox] question about potential null pointer dereference Gustavo A. R. Silva
2017-05-10 15:51 ` Ido Schimmel
2017-05-10 16:01 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox