linux-kernel - Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <29f8532e-324a-4e06-b257-3ef9a037f93f@stanley.mountain>
Date: Mon, 2 Sep 2024 16:12:11 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Baolin Wang <baolin.wang@...ux.alibaba.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Hans de Goede <hdegoede@...hat.com>, linux-kernel@...r.kernel.org,
	platform-driver-x86@...r.kernel.org,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Daniel Scally <djrscally@...il.com>
Subject: Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

On Mon, Sep 02, 2024 at 02:38:18PM +0300, Andy Shevchenko wrote:
> > > I believe the number is only a few at most, which means that you may easily
> > > detect this still with this change being applied, i.e. "anything that
> > > terminates function flow with code 0, passed to dev_err_probe(), is
> > > suspicious".
> > 
> > I think you mean the opposite of what you wrote?  That if we're passing zero to
> > dev_err_probe() and it's the last line in a function it's *NOT* suspicious?
> 
> Yes, sorry, I meant "...terminates function flow _in the middle_...".
> 

I don't think that works.  There are lots of success paths in the middle of
functions.  Smatch already has code to determine whether we should return an
error code or not.

1) Was there a function that returned NULL
2) Was there a function that returned an erorr code/error pointer
3) Was there a bounds check where x >= y?
4) Did we print an error code?
Etc..

I'd end up re-using this code.  This heuristic is more error prone, so there
would be false positives and missed bugs but I can't predict the future so I
don't know how bad it would be.  Looking through the warnings, we still would be
able to detected a number of these because Smatch warnings when you pass NULL to
IS_ERR() or PTR_ERR().

Probably the worse thing from a Smatch perspective is that now I can't just
assume that dev_err_probe() is always an error path.  So for example, we know
that *foo is always initialized on success so we can eliminate all the
"return dev_err_probe();" paths because those are failure path.

I'm never going to like this patch because I always want to make the error paths
more separate and more clear.

regards,
dan carpenter