netdev - Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when phy_select

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210217212411.GC2087@kadam>
Date:   Thu, 18 Feb 2021 00:24:12 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Michael Walle <michael@...le.cc>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: icplus: Call phy_restore_page() when
 phy_select_page() fails

On Wed, Feb 17, 2021 at 06:06:48PM +0100, Andrew Lunn wrote:
> > I'm wondering whether we need to add __acquires() and __releases()
> > annotations to some of these functions so that sparse can catch
> > these cases. Thoughts?
> 
> Hi Russell
> 
> The more tools we have for catching locking problems the better.
> Jakubs patchwork bot should then catch them when a patch is submitted,
> if the developer did not run sparse themselves.

Here is how I wrote the check for Smatch.  The code in the kernel looks
like:

	oldpage = phy_select_page(phydev, 0x0007);

	...

	phy_restore_page(phydev, oldpage, 0);

So what I said is that if phy_select_page() returns an error code then
set "phydev" to &selected state.  Then if we call phy_restore_page()
set it to &undefined.  When we hit a return, check if we have any
"phydev" variables can possibly be in &selected state and print a
warning.

The code is below.

regards,
dan carpenter

#include "smatch.h"
#include "smatch_slist.h"

static int my_id;

STATE(selected);

static sval_t err_min = { .type = &int_ctype, .value = -4095 };
static sval_t err_max = { .type = &int_ctype, .value = -1 };

static void match_phy_select_page(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	set_state(my_id, name, sym, &selected);
}

static void match_phy_restore_page(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	set_state(my_id, name, sym, &undefined);
}

static void match_return(struct expression *expr)
{
	struct sm_state *sm;

	FOR_EACH_MY_SM(my_id, __get_cur_stree(), sm) {
		if (slist_has_state(sm->possible, &selected)) {
			sm_warning("phy_select_page() requires restore on error");
			return;
		}
	} END_FOR_EACH_SM(sm);
}

void check_phy_select_page_fail(int id)
{
	if (option_project != PROJ_KERNEL)
		return;

	my_id = id;

	return_implies_param_key("phy_select_page", err_min, err_max,
				 &match_phy_select_page, 0, "$", NULL);
	add_function_param_key_hook("phy_restore_page", &match_phy_restore_page,
				    0, "$", NULL);
	add_hook(&match_return, RETURN_HOOK);
}