* [net-mellanox] question about potential null pointer dereference
@ 2017-05-10 15:36 ` Gustavo A. R. Silva
0 siblings, 0 replies; 5+ 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!
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 5+ messages in thread* [net-mellanox] question about potential null pointer dereference
@ 2017-05-10 15:36 ` Gustavo A. R. Silva
0 siblings, 0 replies; 5+ 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] 5+ messages in thread* Re: [net-mellanox] question about potential null pointer dereference
2017-05-10 15:36 ` Gustavo A. R. Silva
(?)
@ 2017-05-10 15:51 ` Ido Schimmel
2017-05-10 16:01 ` Gustavo A. R. Silva
-1 siblings, 1 reply; 5+ 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] 5+ 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; 5+ 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. :)
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [net-mellanox] question about potential null pointer dereference
@ 2017-05-10 16:01 ` Gustavo A. R. Silva
0 siblings, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2017-05-10 16:01 UTC | newest]
Thread overview: 5+ 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:36 ` Gustavo A. R. Silva
2017-05-10 15:51 ` Ido Schimmel
2017-05-10 16:01 ` Gustavo A. R. Silva
2017-05-10 16:01 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.