On Mon, Mar 27, 2023 at 07:08:59AM +0000, Steen.Hegelund@xxxxxxxxxxxxx wrote:
> Hi Dan,
>
> I think for now I would like to just remove that line, and then I will try to
> come up with a better test.
Basically all the references to "rule" are invalid after the
vcap_free_rule(rule) line. It's weird that the run time use after free
detectors have not detected this bug.
>
> Not sure about how to add it to KASan test dir? Would that just be a reference
> to this test to avoid bug reporting?
KASan is a use after free detector. We include a bunch of crashing bugs in
that to test if we're detecting use after free bugs.
drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
1442 vcap_enable_lookups(&test_vctrl, &test_netdev, 0, 0,
1443 rule->cookie, false);
^^^^^^^^^^^^
Rule is dereferenced here so it can't be NULL.
1444
1445 vcap_free_rule(rule);
This frees rule. When you free memory, it normally just gets marked as
available. Unless something else allocates/claims it, then it's fine, it still
has the old data in it. There are build options to poison freed (write
garbage values to it) so that use after free bugs are easier to detect but this
is very slow. You can also use KAsan to detect use after frees.
1446
1447 /* Check that the rule has been freed: tricky to access since this
1448 * memory should not be accessible anymore
1449 */
--> 1450 KUNIT_EXPECT_PTR_NE(test, NULL, rule);
^^^^^^^^^^
There is no need to test whether rule is NULL or not. If it were we would
already have crashed. This isn't a dereference so it's harmless.
1451 ret = list_empty(&rule->keyfields);
The list_empty() will dereference rule so this is a use after free.
1452 KUNIT_EXPECT_EQ(test, true, ret);
1453 ret = list_empty(&rule->actionfields);
This is a dereference as well.
1454 KUNIT_EXPECT_EQ(test, true, ret);
1455
1456 vcap_del_rule(&test_vctrl, &test_netdev, id);
regards,
dan carpenter
>
> BR
> Steen
>
> On Sat, 2023-03-25 at 10:35 +0300, Dan Carpenter wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Hello Steen Hegelund,
> >
> > The patch c956b9b318d9: "net: microchip: sparx5: Adding KUNIT tests
> > of key/action values in VCAP API" from Nov 9, 2022, leads to the
> > following Smatch static checker warning:
> >
> > drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c:1416
> > vcap_api_encode_rule_test()
> > warn: 'rule' was already freed.
> >
> > drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
> > 1406
> > 1407 /* Check that the rule has been added */
> > 1408 ret = list_empty(&is2_admin.rules);
> > 1409 KUNIT_EXPECT_EQ(test, false, ret);
> > 1410 KUNIT_EXPECT_EQ(test, 0, ret);
> > 1411 vcap_free_rule(rule);
> > 1412
> > 1413 /* Check that the rule has been freed: tricky to access since
> > this
> > 1414 * memory should not be accessible anymore
> > 1415 */
> > --> 1416 KUNIT_EXPECT_PTR_NE(test, NULL, rule);
> >
> > Obviously the comments say that this is already free so it's going to be
> > "tricky". :P What's happening here? This is to test that KASan will
> > crash properly? Could we put that in the normal KASan tset directory so
> > that we can ignore those deliberate crashing bugs?
> >
> > 1417 ret = list_empty(&rule->keyfields);
> > 1418 KUNIT_EXPECT_EQ(test, true, ret);
> > 1419 ret = list_empty(&rule->actionfields);
> > 1420 KUNIT_EXPECT_EQ(test, true, ret);
> > 1421 }
> >
> > regards,
> > dan carpenter
>
