Potential static analysis ideas

Potential static analysis ideas

[Dan Carpenter]

Rust has many good static analysis features but if we wanted we could
implement a number of stricter rules in C.  With Smatch I have tried to
focus on exclusively on finding bugs because everyone can agree that
bugs are bad.  But if some subsystems wanted to implement stricter rules
just as a hardenning measure then that's a doable thing.

For example, I've tried a bunch of approaches to warning about when the
user can trigger an integer overflow.  The challenge is that most
integer overflows are harmless and do not cause a real life bug.

I've also written some checks for locking bugs where the warning is
"expected lock 'foo' held when accessing struct member 'bar'".  The
problem is that I'm not a locking expert.  My process is 1) Infer the
pairing between this lock protect this pointer.  2)  Make list of places
where the pointer holds a function pointer and we free it under lock
(I still need to add the locking part).  3) Make a list of places where
we access the data without holding the lock and the attacker can control
the timing with userfaultd or whatever.

This sort of locking check which finds real life exploitable bugs is
much more complicated to write.  If there were subsystems which had
simpler, hardenned rules then that would be easier.

Another idea is that if I were a subsystem maintainer, I think I would
enforce my prefered error handling rules.  1) Every allocation function
must have exactly one free function.  2) Every function should clean up
after itself on failure. 3) Never free things that aren't allocated.
4) The frees must happen in reverse order from how they were allocated.

These days we have disabled GCC's uninitialized warnings so it falls
to static analysis devs to review all the new warnings.  I sometimes
ignore warnings if they look like probably they aren't a bug.  Does this
function allow zero size writes?  Does this switch statement really need
a default case?  Maybe sometimes I miss bugs.

Here is another example of something which I have a check for but I
haven't pushed.

	ret = frob();
	if (!ret)
		return ret;

This code is normally correct but sometimes it means the if statement is
reversed and it should be "if (ret) return ret;".  To me returning a
literal is always more clear but but clearly the original author felt
"ret" was more clear...  It's only a few bugs per year so it's not a big
deal either way.

A sort of related example that I have pushed is this:

	int ret = 0;

	/* 20 lines of code */

	if (condition)
		goto cleanup;

These trigger a published Smatch warning for missing error codes but I
only email people when I can't understand the code.  Please, put the
"ret = 0;" within 4 lines of the goto.

Those are just some ideas of things we could do if we wanted to use
static analysis for hardenning instead of for fixing bugs.

regards,
dan carpenter

Re: Potential static analysis ideas

[Geert Uytterhoeven]

Hi Dan,

On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> These days we have disabled GCC's uninitialized warnings so it falls
> to static analysis devs to review all the new warnings.  I sometimes
> ignore warnings if they look like probably they aren't a bug.  Does this
> function allow zero size writes?  Does this switch statement really need
> a default case?  Maybe sometimes I miss bugs.

Yeah, I tended to find a few real bugs every release using the good old
gcc 4.2, before support for it was dropped.
I hope someone still runs modern compilers with GCC's uninitialized
warnings enabled again?

> Here is another example of something which I have a check for but I
> haven't pushed.
>
>         ret = frob();
>         if (!ret)
>                 return ret;
>
> This code is normally correct but sometimes it means the if statement is
> reversed and it should be "if (ret) return ret;".  To me returning a
> literal is always more clear but but clearly the original author felt
> "ret" was more clear...  It's only a few bugs per year so it's not a big
> deal either way.

To make it work well, you need to know if frob() and/or the current
function return an error code or not.  While you can use some heuristics
(e.g. is there any return -Exxx), perhaps we can add an annotation to
indicate if a function returns an error code, or an error pointer?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Potential static analysis ideas

[Julia Lawall]

On Sat, 24 Jul 2021, Geert Uytterhoeven wrote:

> Hi Dan,
>
> On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > These days we have disabled GCC's uninitialized warnings so it falls
> > to static analysis devs to review all the new warnings.  I sometimes
> > ignore warnings if they look like probably they aren't a bug.  Does this
> > function allow zero size writes?  Does this switch statement really need
> > a default case?  Maybe sometimes I miss bugs.
>
> Yeah, I tended to find a few real bugs every release using the good old
> gcc 4.2, before support for it was dropped.
> I hope someone still runs modern compilers with GCC's uninitialized
> warnings enabled again?
>
> > Here is another example of something which I have a check for but I
> > haven't pushed.
> >
> >         ret = frob();
> >         if (!ret)
> >                 return ret;
> >
> > This code is normally correct but sometimes it means the if statement is
> > reversed and it should be "if (ret) return ret;".  To me returning a
> > literal is always more clear but but clearly the original author felt
> > "ret" was more clear...  It's only a few bugs per year so it's not a big
> > deal either way.
>
> To make it work well, you need to know if frob() and/or the current
> function return an error code or not.  While you can use some heuristics
> (e.g. is there any return -Exxx), perhaps we can add an annotation to
> indicate if a function returns an error code, or an error pointer?

FWIW, the ability of Coccinelle to add and match against annotations is
currently being greatly improved by Keisuke Nishimura.

julia

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: Potential static analysis ideas

[Arnd Bergmann]

On Sat, Jul 24, 2021 at 3:33 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Dan,
>
> On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > These days we have disabled GCC's uninitialized warnings so it falls
> > to static analysis devs to review all the new warnings.  I sometimes
> > ignore warnings if they look like probably they aren't a bug.  Does this
> > function allow zero size writes?  Does this switch statement really need
> > a default case?  Maybe sometimes I miss bugs.
>
> Yeah, I tended to find a few real bugs every release using the good old
> gcc 4.2, before support for it was dropped.
> I hope someone still runs modern compilers with GCC's uninitialized
> warnings enabled again?

Unfortunately, the new inlining in gcc-10 and up has made the
Wmaybe-uninitialized option pretty much unusable, especially since
all the uninitialized_var() annotations are gone. I tried for a while to
keep that running on my randconfig build machine, but the number of
warnings that got added was overwhelming after a while.

This does contain a number of real bugs, and at least clang and
smatch can both catch a notable subset of those, but it would be
good to have a replacement that warns developers about not
adding uninitialized variable uses in the first place.

    Arnd

Re: Potential static analysis ideas

[Laurent Pinchart]

On Sat, Jul 24, 2021 at 03:33:48PM +0200, Geert Uytterhoeven wrote:
> On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter wrote:
> > These days we have disabled GCC's uninitialized warnings so it falls
> > to static analysis devs to review all the new warnings.  I sometimes
> > ignore warnings if they look like probably they aren't a bug.  Does this
> > function allow zero size writes?  Does this switch statement really need
> > a default case?  Maybe sometimes I miss bugs.
> 
> Yeah, I tended to find a few real bugs every release using the good old
> gcc 4.2, before support for it was dropped.
> I hope someone still runs modern compilers with GCC's uninitialized
> warnings enabled again?
> 
> > Here is another example of something which I have a check for but I
> > haven't pushed.
> >
> >         ret = frob();
> >         if (!ret)
> >                 return ret;
> >
> > This code is normally correct but sometimes it means the if statement is
> > reversed and it should be "if (ret) return ret;".  To me returning a
> > literal is always more clear but but clearly the original author felt
> > "ret" was more clear...  It's only a few bugs per year so it's not a big
> > deal either way.
> 
> To make it work well, you need to know if frob() and/or the current
> function return an error code or not.  While you can use some heuristics
> (e.g. is there any return -Exxx), perhaps we can add an annotation to
> indicate if a function returns an error code, or an error pointer?

https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/

I think it would be useful, if not for the tools, at least for
developers.

-- 
Regards,

Laurent Pinchart

Re: Potential static analysis ideas

[NeilBrown]

On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> On Sat, Jul 24, 2021 at 03:33:48PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter wrote:
> > > These days we have disabled GCC's uninitialized warnings so it falls
> > > to static analysis devs to review all the new warnings.  I sometimes
> > > ignore warnings if they look like probably they aren't a bug.  Does this
> > > function allow zero size writes?  Does this switch statement really need
> > > a default case?  Maybe sometimes I miss bugs.
> > 
> > Yeah, I tended to find a few real bugs every release using the good old
> > gcc 4.2, before support for it was dropped.
> > I hope someone still runs modern compilers with GCC's uninitialized
> > warnings enabled again?
> > 
> > > Here is another example of something which I have a check for but I
> > > haven't pushed.
> > >
> > >         ret = frob();
> > >         if (!ret)
> > >                 return ret;
> > >
> > > This code is normally correct but sometimes it means the if statement is
> > > reversed and it should be "if (ret) return ret;".  To me returning a
> > > literal is always more clear but but clearly the original author felt
> > > "ret" was more clear...  It's only a few bugs per year so it's not a big
> > > deal either way.
> > 
> > To make it work well, you need to know if frob() and/or the current
> > function return an error code or not.  While you can use some heuristics
> > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > indicate if a function returns an error code, or an error pointer?
> 
> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> 
> I think it would be useful, if not for the tools, at least for
> developers.

Agreed.  I added some code to smatch so that I could annotate pointers to
say if they are allowed to be NULL.  The implementation isn't perfect,
but I love having that extra documentation about when I do or don't have
to check for NULL.

NeilBrown

Re: Potential static analysis ideas

[Dan Carpenter]

On Sat, Jul 24, 2021 at 03:33:48PM +0200, Geert Uytterhoeven wrote:
> > Here is another example of something which I have a check for but I
> > haven't pushed.
> >
> >         ret = frob();
> >         if (!ret)
> >                 return ret;
> >
> > This code is normally correct but sometimes it means the if statement is
> > reversed and it should be "if (ret) return ret;".  To me returning a
> > literal is always more clear but but clearly the original author felt
> > "ret" was more clear...  It's only a few bugs per year so it's not a big
> > deal either way.
> 
> To make it work well, you need to know if frob() and/or the current
> function return an error code or not.  While you can use some heuristics
> (e.g. is there any return -Exxx), perhaps we can add an annotation to
> indicate if a function returns an error code, or an error pointer?

The function is normal return zero or error codes, but the question is
do we want to return for success or for failure.  This kind of bug is
normally caught very early in testing but some of the big refactor
patches can't be tested on every hardware.

regards,
dan carpenter

Re: Potential static analysis ideas

[Arnd Bergmann]

On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > >
> > > To make it work well, you need to know if frob() and/or the current
> > > function return an error code or not.  While you can use some heuristics
> > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > indicate if a function returns an error code, or an error pointer?
> >
> > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> >
> > I think it would be useful, if not for the tools, at least for
> > developers.
>
> Agreed.  I added some code to smatch so that I could annotate pointers to
> say if they are allowed to be NULL.  The implementation isn't perfect,
> but I love having that extra documentation about when I do or don't have
> to check for NULL.

I can think of four different annotations that limit what a pointer return from
a function can be:

a) either a valid pointer or NULL, but never an error pointer,
b) either a valid pointer or an error pointer, but not NULL,
c) always a valid pointer, never NULL or an error,
d) always NULL, but callers are expected to check for error pointers.

The last one is the tricky bit because there are a couple of subsystems
that intentionally return an opaque NULL pointer to indicate success
when the subsystem is compile-time disabled, and expect callers to not
dereference that pointer but instead pass it into other stubbed-out
interfaces. I think this one is rather specific to the kernel, but also really
useful to have in a static checker because everyone gets those wrong.

        Arnd

Re: Potential static analysis ideas

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > > To make it work well, you need to know if frob() and/or the current
> > > > function return an error code or not.  While you can use some heuristics
> > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > indicate if a function returns an error code, or an error pointer?
> > >
> > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > >
> > > I think it would be useful, if not for the tools, at least for
> > > developers.
> >
> > Agreed.  I added some code to smatch so that I could annotate pointers to
> > say if they are allowed to be NULL.  The implementation isn't perfect,
> > but I love having that extra documentation about when I do or don't have
> > to check for NULL.
>
> I can think of four different annotations that limit what a pointer return from
> a function can be:
>
> a) either a valid pointer or NULL, but never an error pointer,
> b) either a valid pointer or an error pointer, but not NULL,
> c) always a valid pointer, never NULL or an error,
> d) always NULL, but callers are expected to check for error pointers.

e) either a valid pointer, NULL, or an error pointer

The last pattern is seen with the various *get*_optional() functions.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Potential static analysis ideas

[Arnd Bergmann]

On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> > > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > > > To make it work well, you need to know if frob() and/or the current
> > > > > function return an error code or not.  While you can use some heuristics
> > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > > indicate if a function returns an error code, or an error pointer?
> > > >
> > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > > >
> > > > I think it would be useful, if not for the tools, at least for
> > > > developers.
> > >
> > > Agreed.  I added some code to smatch so that I could annotate pointers to
> > > say if they are allowed to be NULL.  The implementation isn't perfect,
> > > but I love having that extra documentation about when I do or don't have
> > > to check for NULL.
> >
> > I can think of four different annotations that limit what a pointer return from
> > a function can be:
> >
> > a) either a valid pointer or NULL, but never an error pointer,
> > b) either a valid pointer or an error pointer, but not NULL,
> > c) always a valid pointer, never NULL or an error,
> > d) always NULL, but callers are expected to check for error pointers.
>
> e) either a valid pointer, NULL, or an error pointer
>
> The last pattern is seen with the various *get*_optional() functions.

I would always consider those the exact bug that I meant with "because
everyone gets those wrong". I think the idea of the "optional" functions is
that you have two implementations b) and d) and pick one of them
at compile time. To the caller this means either an error pointer or
success, but checking for NULL is a bug in the caller, while conditionally
returning NULL or ERR_PTR() would be a bug in the interface.

     Arnd

Re: Potential static analysis ideas

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Jul 26, 2021 at 10:21 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > > > > To make it work well, you need to know if frob() and/or the current
> > > > > > function return an error code or not.  While you can use some heuristics
> > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > > > indicate if a function returns an error code, or an error pointer?
> > > > >
> > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > > > >
> > > > > I think it would be useful, if not for the tools, at least for
> > > > > developers.
> > > >
> > > > Agreed.  I added some code to smatch so that I could annotate pointers to
> > > > say if they are allowed to be NULL.  The implementation isn't perfect,
> > > > but I love having that extra documentation about when I do or don't have
> > > > to check for NULL.
> > >
> > > I can think of four different annotations that limit what a pointer return from
> > > a function can be:
> > >
> > > a) either a valid pointer or NULL, but never an error pointer,
> > > b) either a valid pointer or an error pointer, but not NULL,
> > > c) always a valid pointer, never NULL or an error,
> > > d) always NULL, but callers are expected to check for error pointers.
> >
> > e) either a valid pointer, NULL, or an error pointer
> >
> > The last pattern is seen with the various *get*_optional() functions.
>
> I would always consider those the exact bug that I meant with "because
> everyone gets those wrong". I think the idea of the "optional" functions is
> that you have two implementations b) and d) and pick one of them
> at compile time. To the caller this means either an error pointer or
> success, but checking for NULL is a bug in the caller, while conditionally
> returning NULL or ERR_PTR() would be a bug in the interface.

e) is not for optional kernel features, but for optional hardware
functionality.  So it is not known at compile time, but depends on
e.g. the DTB.  See e.g. devm_clk_get_optional().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Potential static analysis ideas

[Arnd Bergmann]

On Mon, Jul 26, 2021 at 10:39 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Jul 26, 2021 at 10:21 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > e) either a valid pointer, NULL, or an error pointer
> > >
> > > The last pattern is seen with the various *get*_optional() functions.
> >
> > I would always consider those the exact bug that I meant with "because
> > everyone gets those wrong". I think the idea of the "optional" functions is
> > that you have two implementations b) and d) and pick one of them
> > at compile time. To the caller this means either an error pointer or
> > success, but checking for NULL is a bug in the caller, while conditionally
> > returning NULL or ERR_PTR() would be a bug in the interface.
>
> e) is not for optional kernel features, but for optional hardware
> functionality.  So it is not known at compile time, but depends on
> e.g. the DTB.  See e.g. devm_clk_get_optional().

Ah right, I forgot we had those as well. However, I suppose no amount
of annotation is going to help with those, as far as the checker is concerned,
I would just leave this as not annotated. At least with the clk API,
the callers are not able to dereference the pointer because the definition
of 'struct clk' is private to drivers/clk/clk.c.

        Arnd

Re: Potential static analysis ideas

[Julia Lawall]

On Mon, 26 Jul 2021, Arnd Bergmann wrote:

> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > > > > To make it work well, you need to know if frob() and/or the current
> > > > > > function return an error code or not.  While you can use some heuristics
> > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > > > indicate if a function returns an error code, or an error pointer?
> > > > >
> > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > > > >
> > > > > I think it would be useful, if not for the tools, at least for
> > > > > developers.
> > > >
> > > > Agreed.  I added some code to smatch so that I could annotate pointers to
> > > > say if they are allowed to be NULL.  The implementation isn't perfect,
> > > > but I love having that extra documentation about when I do or don't have
> > > > to check for NULL.
> > >
> > > I can think of four different annotations that limit what a pointer return from
> > > a function can be:
> > >
> > > a) either a valid pointer or NULL, but never an error pointer,
> > > b) either a valid pointer or an error pointer, but not NULL,
> > > c) always a valid pointer, never NULL or an error,
> > > d) always NULL, but callers are expected to check for error pointers.
> >
> > e) either a valid pointer, NULL, or an error pointer
> >
> > The last pattern is seen with the various *get*_optional() functions.
>
> I would always consider those the exact bug that I meant with "because
> everyone gets those wrong". I think the idea of the "optional" functions is
> that you have two implementations b) and d) and pick one of them
> at compile time. To the caller this means either an error pointer or
> success, but checking for NULL is a bug in the caller, while conditionally
> returning NULL or ERR_PTR() would be a bug in the interface.

I'm not sure to understand the "bug in the caller" part.  Couldn't there
be two possible definitions of the called function, according to different
configuration options, and a single caller that calls both?

Also, over 230 files contain functions that return both NULL and ERR_PTR.
A random example, chosen for conciseness, is the following from
fs/overlayfs/inode.c:

struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
                               bool is_upper)
{
 	struct inode *inode, *key = d_inode(real);

        inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
	if (!inode)
                return NULL;

        if (!ovl_verify_inode(inode, is_upper ? NULL : real,
                              is_upper ? real : NULL, false)) {
                iput(inode);
                return ERR_PTR(-ESTALE);
        }

        return inode;
}

julia

Re: Potential static analysis ideas

[Hannes Reinecke]

On 7/26/21 10:55 AM, Julia Lawall wrote:
> 
> 
> On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> 
>> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
>>>>> On Sun, 25 Jul 2021, Laurent Pinchart wrote:
>>>>>>> To make it work well, you need to know if frob() and/or the current
>>>>>>> function return an error code or not.  While you can use some heuristics
>>>>>>> (e.g. is there any return -Exxx), perhaps we can add an annotation to
>>>>>>> indicate if a function returns an error code, or an error pointer?
>>>>>>
>>>>>> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
>>>>>>
>>>>>> I think it would be useful, if not for the tools, at least for
>>>>>> developers.
>>>>>
>>>>> Agreed.  I added some code to smatch so that I could annotate pointers to
>>>>> say if they are allowed to be NULL.  The implementation isn't perfect,
>>>>> but I love having that extra documentation about when I do or don't have
>>>>> to check for NULL.
>>>>
>>>> I can think of four different annotations that limit what a pointer return from
>>>> a function can be:
>>>>
>>>> a) either a valid pointer or NULL, but never an error pointer,
>>>> b) either a valid pointer or an error pointer, but not NULL,
>>>> c) always a valid pointer, never NULL or an error,
>>>> d) always NULL, but callers are expected to check for error pointers.
>>>
>>> e) either a valid pointer, NULL, or an error pointer
>>>
>>> The last pattern is seen with the various *get*_optional() functions.
>>
>> I would always consider those the exact bug that I meant with "because
>> everyone gets those wrong". I think the idea of the "optional" functions is
>> that you have two implementations b) and d) and pick one of them
>> at compile time. To the caller this means either an error pointer or
>> success, but checking for NULL is a bug in the caller, while conditionally
>> returning NULL or ERR_PTR() would be a bug in the interface.
> 
> I'm not sure to understand the "bug in the caller" part.  Couldn't there
> be two possible definitions of the called function, according to different
> configuration options, and a single caller that calls both?
> 
> Also, over 230 files contain functions that return both NULL and ERR_PTR.
> A random example, chosen for conciseness, is the following from
> fs/overlayfs/inode.c:
> 
> struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>                                 bool is_upper)
> {
>   	struct inode *inode, *key = d_inode(real);
> 
>          inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
> 	if (!inode)
>                  return NULL;
> 
>          if (!ovl_verify_inode(inode, is_upper ? NULL : real,
>                                is_upper ? real : NULL, false)) {
>                  iput(inode);
>                  return ERR_PTR(-ESTALE);
>          }
> 
>          return inode;
> }
> 
And that I would consider a coding error.
If a function is able to return an error pointer it should _always_ 
return an error pointer; here it would be trivial to return -ENXIO 
instead of NULL in the first condition.

Not doing so is just sloppy programming IMO.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: Potential static analysis ideas

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Jul 26, 2021 at 10:52 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jul 26, 2021 at 10:39 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Mon, Jul 26, 2021 at 10:21 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > >
> > > > e) either a valid pointer, NULL, or an error pointer
> > > >
> > > > The last pattern is seen with the various *get*_optional() functions.
> > >
> > > I would always consider those the exact bug that I meant with "because
> > > everyone gets those wrong". I think the idea of the "optional" functions is
> > > that you have two implementations b) and d) and pick one of them
> > > at compile time. To the caller this means either an error pointer or
> > > success, but checking for NULL is a bug in the caller, while conditionally
> > > returning NULL or ERR_PTR() would be a bug in the interface.
> >
> > e) is not for optional kernel features, but for optional hardware
> > functionality.  So it is not known at compile time, but depends on
> > e.g. the DTB.  See e.g. devm_clk_get_optional().
>
> Ah right, I forgot we had those as well. However, I suppose no amount
> of annotation is going to help with those, as far as the checker is concerned,
> I would just leave this as not annotated. At least with the clk API,
> the callers are not able to dereference the pointer because the definition
> of 'struct clk' is private to drivers/clk/clk.c.

And the clk API supports calling further clock ops with a NULL pointer,
which eases handling optional clocks.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Potential static analysis ideas

[Dan Carpenter]

On Mon, Jul 26, 2021 at 10:20:45AM +0200, Arnd Bergmann wrote:
> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > > > > To make it work well, you need to know if frob() and/or the current
> > > > > > function return an error code or not.  While you can use some heuristics
> > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > > > indicate if a function returns an error code, or an error pointer?
> > > > >
> > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > > > >
> > > > > I think it would be useful, if not for the tools, at least for
> > > > > developers.
> > > >
> > > > Agreed.  I added some code to smatch so that I could annotate pointers to
> > > > say if they are allowed to be NULL.  The implementation isn't perfect,
> > > > but I love having that extra documentation about when I do or don't have
> > > > to check for NULL.
> > >
> > > I can think of four different annotations that limit what a pointer return from
> > > a function can be:
> > >
> > > a) either a valid pointer or NULL, but never an error pointer,
> > > b) either a valid pointer or an error pointer, but not NULL,
> > > c) always a valid pointer, never NULL or an error,
> > > d) always NULL, but callers are expected to check for error pointers.
> >
> > e) either a valid pointer, NULL, or an error pointer
> >
> > The last pattern is seen with the various *get*_optional() functions.
> 
> I would always consider those the exact bug that I meant with "because
> everyone gets those wrong". I think the idea of the "optional" functions is
> that you have two implementations b) and d) and pick one of them
> at compile time. To the caller this means either an error pointer or
> success, but checking for NULL is a bug in the caller, while conditionally
> returning NULL or ERR_PTR() would be a bug in the interface.
> 
>      Arnd

When a function returns both error pointers and NULL then the NULL just
means the feature is diliberately disabled.  So don't print an error,
but NULL returns still have to be handled.  The other mistake I see with
this is when the function returns an error pointer the caller just says
this is optional so let's continue.

	p = get_optional();
	if (IS_ERR(p))
		p = NULL;

It might be helpful if it lets the system boot but generally errors
should be reported to the user.

regards,
dan carpenter

Re: Potential static analysis ideas

[Geert Uytterhoeven]

Hi Hannes,

On Mon, Jul 26, 2021 at 11:08 AM Hannes Reinecke <hare@suse.de> wrote:
> On 7/26/21 10:55 AM, Julia Lawall wrote:
> > On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> >> On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >>>> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> >>>>> On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> >>>>>>> To make it work well, you need to know if frob() and/or the current
> >>>>>>> function return an error code or not.  While you can use some heuristics
> >>>>>>> (e.g. is there any return -Exxx), perhaps we can add an annotation to
> >>>>>>> indicate if a function returns an error code, or an error pointer?
> >>>>>>
> >>>>>> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> >>>>>>
> >>>>>> I think it would be useful, if not for the tools, at least for
> >>>>>> developers.
> >>>>>
> >>>>> Agreed.  I added some code to smatch so that I could annotate pointers to
> >>>>> say if they are allowed to be NULL.  The implementation isn't perfect,
> >>>>> but I love having that extra documentation about when I do or don't have
> >>>>> to check for NULL.
> >>>>
> >>>> I can think of four different annotations that limit what a pointer return from
> >>>> a function can be:
> >>>>
> >>>> a) either a valid pointer or NULL, but never an error pointer,
> >>>> b) either a valid pointer or an error pointer, but not NULL,
> >>>> c) always a valid pointer, never NULL or an error,
> >>>> d) always NULL, but callers are expected to check for error pointers.
> >>>
> >>> e) either a valid pointer, NULL, or an error pointer
> >>>
> >>> The last pattern is seen with the various *get*_optional() functions.
> >>
> >> I would always consider those the exact bug that I meant with "because
> >> everyone gets those wrong". I think the idea of the "optional" functions is
> >> that you have two implementations b) and d) and pick one of them
> >> at compile time. To the caller this means either an error pointer or
> >> success, but checking for NULL is a bug in the caller, while conditionally
> >> returning NULL or ERR_PTR() would be a bug in the interface.
> >
> > I'm not sure to understand the "bug in the caller" part.  Couldn't there
> > be two possible definitions of the called function, according to different
> > configuration options, and a single caller that calls both?
> >
> > Also, over 230 files contain functions that return both NULL and ERR_PTR.
> > A random example, chosen for conciseness, is the following from
> > fs/overlayfs/inode.c:
> >
> > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
> >                                 bool is_upper)
> > {
> >       struct inode *inode, *key = d_inode(real);
> >
> >          inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
> >       if (!inode)
> >                  return NULL;
> >
> >          if (!ovl_verify_inode(inode, is_upper ? NULL : real,
> >                                is_upper ? real : NULL, false)) {
> >                  iput(inode);
> >                  return ERR_PTR(-ESTALE);
> >          }
> >
> >          return inode;
> > }
> >
> And that I would consider a coding error.
> If a function is able to return an error pointer it should _always_
> return an error pointer; here it would be trivial to return -ENXIO
> instead of NULL in the first condition.
>
> Not doing so is just sloppy programming IMO.

In this case I agree.

For optional resources, the ability to return NULL (and for the caller
not having to care if the check should be for -ENOENT, -ENODEV,
or something else), and accept NULL for further operations greatly
simplifies the handling of optional resources like clocks, enable
gpios, resets, regulators, ...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Potential static analysis ideas

[Dan Carpenter]

On Mon, Jul 26, 2021 at 11:08:01AM +0200, Hannes Reinecke wrote:
> And that I would consider a coding error.
> If a function is able to return an error pointer it should _always_ return
> an error pointer; here it would be trivial to return -ENXIO instead of NULL
> in the first condition.
> 
> Not doing so is just sloppy programming IMO.
> 
> Cheers,

But it's not an error.  One place where this bit me recently was I was
trying to debug some code and I added:

	if (err)
		printk("function blah returned err");

But their list iterator returns an error code when it got to the end of
the list so I was flooded with errors.  It be nicer "if we can't
allocate enough memory for the next item return an error pointer.  If we
come to the end of the loop then return NULL"

regards,
dan carpenter

Re: Potential static analysis ideas

[Julia Lawall]

> > > Also, over 230 files contain functions that return both NULL and ERR_PTR.
> > > A random example, chosen for conciseness, is the following from
> > > fs/overlayfs/inode.c:
> > >
> > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
> > >                                 bool is_upper)
> > > {
> > >       struct inode *inode, *key = d_inode(real);
> > >
> > >          inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
> > >       if (!inode)
> > >                  return NULL;
> > >
> > >          if (!ovl_verify_inode(inode, is_upper ? NULL : real,
> > >                                is_upper ? real : NULL, false)) {
> > >                  iput(inode);
> > >                  return ERR_PTR(-ESTALE);
> > >          }
> > >
> > >          return inode;
> > > }
> > >
> > And that I would consider a coding error.
> > If a function is able to return an error pointer it should _always_
> > return an error pointer; here it would be trivial to return -ENXIO
> > instead of NULL in the first condition.
> >
> > Not doing so is just sloppy programming IMO.
>
> In this case I agree.

I looked at the calling context, and it is not so obvious.  There are
different behaviors in the two cases at both callsites.  It is like what
Dan describes.  If the thing is not available, we just move on.  If it is
available some action is needed.  If there is an actual error, some error
handling is needed.

julia

>
> For optional resources, the ability to return NULL (and for the caller
> not having to care if the check should be for -ENOENT, -ENODEV,
> or something else), and accept NULL for further operations greatly
> simplifies the handling of optional resources like clocks, enable
> gpios, resets, regulators, ...
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>
>

Re: Potential static analysis ideas

[Hannes Reinecke]

On 7/26/21 11:28 AM, Julia Lawall wrote:
>>>> Also, over 230 files contain functions that return both NULL and ERR_PTR.
>>>> A random example, chosen for conciseness, is the following from
>>>> fs/overlayfs/inode.c:
>>>>
>>>> struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>>>>                                  bool is_upper)
>>>> {
>>>>        struct inode *inode, *key = d_inode(real);
>>>>
>>>>           inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
>>>>        if (!inode)
>>>>                   return NULL;
>>>>
>>>>           if (!ovl_verify_inode(inode, is_upper ? NULL : real,
>>>>                                 is_upper ? real : NULL, false)) {
>>>>                   iput(inode);
>>>>                   return ERR_PTR(-ESTALE);
>>>>           }
>>>>
>>>>           return inode;
>>>> }
>>>>
>>> And that I would consider a coding error.
>>> If a function is able to return an error pointer it should _always_
>>> return an error pointer; here it would be trivial to return -ENXIO
>>> instead of NULL in the first condition.
>>>
>>> Not doing so is just sloppy programming IMO.
>>
>> In this case I agree.
> 
> I looked at the calling context, and it is not so obvious.  There are
> different behaviors in the two cases at both callsites.  It is like what
> Dan describes.  If the thing is not available, we just move on.  If it is
> available some action is needed.  If there is an actual error, some error
> handling is needed.
> 
But isn't 'not available' an error, too?
And isn't that exactly why we have individual errnos?

Why do we have to introduce different classes of errors (errno, NULL 
pointer), when we could have used a simple errno lik -ENXIO?
And, of course, documentation for that function outlining what exactly 
the meaning of the individual error numbers is.
But then we would need that anyway to clarify how the caller should 
interpret a 'NULL' return value.

So: not convinced :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: Potential static analysis ideas

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2641 bytes --]



On Mon, 26 Jul 2021, Hannes Reinecke wrote:

> On 7/26/21 11:28 AM, Julia Lawall wrote:
> > > > > Also, over 230 files contain functions that return both NULL and
> > > > > ERR_PTR.
> > > > > A random example, chosen for conciseness, is the following from
> > > > > fs/overlayfs/inode.c:
> > > > >
> > > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry
> > > > > *real,
> > > > >                                  bool is_upper)
> > > > > {
> > > > >        struct inode *inode, *key = d_inode(real);
> > > > >
> > > > >           inode = ilookup5(sb, (unsigned long) key, ovl_inode_test,
> > > > > key);
> > > > >        if (!inode)
> > > > >                   return NULL;
> > > > >
> > > > >           if (!ovl_verify_inode(inode, is_upper ? NULL : real,
> > > > >                                 is_upper ? real : NULL, false)) {
> > > > >                   iput(inode);
> > > > >                   return ERR_PTR(-ESTALE);
> > > > >           }
> > > > >
> > > > >           return inode;
> > > > > }
> > > > >
> > > > And that I would consider a coding error.
> > > > If a function is able to return an error pointer it should _always_
> > > > return an error pointer; here it would be trivial to return -ENXIO
> > > > instead of NULL in the first condition.
> > > >
> > > > Not doing so is just sloppy programming IMO.
> > >
> > > In this case I agree.
> >
> > I looked at the calling context, and it is not so obvious.  There are
> > different behaviors in the two cases at both callsites.  It is like what
> > Dan describes.  If the thing is not available, we just move on.  If it is
> > available some action is needed.  If there is an actual error, some error
> > handling is needed.
> >
> But isn't 'not available' an error, too?

No, it doesn't seem to be.

> And isn't that exactly why we have individual errnos?
>
> Why do we have to introduce different classes of errors (errno, NULL pointer),
> when we could have used a simple errno lik -ENXIO?
> And, of course, documentation for that function outlining what exactly the
> meaning of the individual error numbers is.
> But then we would need that anyway to clarify how the caller should interpret
> a 'NULL' return value.
>
> So: not convinced :-)

OK, I basically agree.  The point is just that there are going to be two
tests at both call sites either way.

julia

> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                Kernel Storage Architect
> hare@suse.de                              +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
>

Re: Potential static analysis ideas

[Paul E. McKenney]

On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote:
> Rust has many good static analysis features but if we wanted we could
> implement a number of stricter rules in C.  With Smatch I have tried to
> focus on exclusively on finding bugs because everyone can agree that
> bugs are bad.  But if some subsystems wanted to implement stricter rules
> just as a hardenning measure then that's a doable thing.
> 
> For example, I've tried a bunch of approaches to warning about when the
> user can trigger an integer overflow.  The challenge is that most
> integer overflows are harmless and do not cause a real life bug.

I would not want overflow checks for unsigned integers, but it might
be helpful for signed integers.  But yes, most of us rely on fwrapv,
so that kernelwide checks for signed integer overflow will be quite noisy.

							Thanx, Paul

Re: Potential static analysis ideas

[James Bottomley]

On Mon, 2021-07-26 at 11:16 +0200, Geert Uytterhoeven wrote:
> Hi Hannes,
> 
> On Mon, Jul 26, 2021 at 11:08 AM Hannes Reinecke <hare@suse.de>
> wrote:
> > On 7/26/21 10:55 AM, Julia Lawall wrote:
> > > On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> > > > On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven <
> > > > geert@linux-m68k.org> wrote:
> > > > > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann <arnd@arndb.de>
> > > > > wrote:
> > > > > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de>
> > > > > > wrote:
> > > > > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > > > > > > > To make it work well, you need to know if frob()
> > > > > > > > > and/or the current
> > > > > > > > > function return an error code or not.  While you can
> > > > > > > > > use some heuristics
> > > > > > > > > (e.g. is there any return -Exxx), perhaps we can add
> > > > > > > > > an annotation to
> > > > > > > > > indicate if a function returns an error code, or an
> > > > > > > > > error pointer?
> > > > > > > > 
> > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > > > > > > > 
> > > > > > > > I think it would be useful, if not for the tools, at
> > > > > > > > least for
> > > > > > > > developers.
> > > > > > > 
> > > > > > > Agreed.  I added some code to smatch so that I could
> > > > > > > annotate pointers to
> > > > > > > say if they are allowed to be NULL.  The implementation
> > > > > > > isn't perfect,
> > > > > > > but I love having that extra documentation about when I
> > > > > > > do or don't have
> > > > > > > to check for NULL.
> > > > > > 
> > > > > > I can think of four different annotations that limit what a
> > > > > > pointer return from
> > > > > > a function can be:
> > > > > > 
> > > > > > a) either a valid pointer or NULL, but never an error
> > > > > > pointer,
> > > > > > b) either a valid pointer or an error pointer, but not
> > > > > > NULL,
> > > > > > c) always a valid pointer, never NULL or an error,
> > > > > > d) always NULL, but callers are expected to check for error
> > > > > > pointers.
> > > > > 
> > > > > e) either a valid pointer, NULL, or an error pointer
> > > > > 
> > > > > The last pattern is seen with the various *get*_optional()
> > > > > functions.
> > > > 
> > > > I would always consider those the exact bug that I meant with
> > > > "because
> > > > everyone gets those wrong". I think the idea of the "optional"
> > > > functions is
> > > > that you have two implementations b) and d) and pick one of
> > > > them
> > > > at compile time. To the caller this means either an error
> > > > pointer or
> > > > success, but checking for NULL is a bug in the caller, while
> > > > conditionally
> > > > returning NULL or ERR_PTR() would be a bug in the interface.
> > > 
> > > I'm not sure to understand the "bug in the caller"
> > > part.  Couldn't there
> > > be two possible definitions of the called function, according to
> > > different
> > > configuration options, and a single caller that calls both?
> > > 
> > > Also, over 230 files contain functions that return both NULL and
> > > ERR_PTR.
> > > A random example, chosen for conciseness, is the following from
> > > fs/overlayfs/inode.c:
> > > 
> > > struct inode *ovl_lookup_inode(struct super_block *sb, struct
> > > dentry *real,
> > >                                 bool is_upper)
> > > {
> > >       struct inode *inode, *key = d_inode(real);
> > > 
> > >          inode = ilookup5(sb, (unsigned long) key,
> > > ovl_inode_test, key);
> > >       if (!inode)
> > >                  return NULL;
> > > 
> > >          if (!ovl_verify_inode(inode, is_upper ? NULL : real,
> > >                                is_upper ? real : NULL, false)) {
> > >                  iput(inode);
> > >                  return ERR_PTR(-ESTALE);
> > >          }
> > > 
> > >          return inode;
> > > }
> > > 
> > And that I would consider a coding error.
> > If a function is able to return an error pointer it should _always_
> > return an error pointer; here it would be trivial to return -ENXIO
> > instead of NULL in the first condition.
> > 
> > Not doing so is just sloppy programming IMO.
> 
> In this case I agree.

Actually, I don't think so ... we have NULL return all over the inode
and dentry code.  It's a legitimate return for "I couldn't find what
you asked for" or in the dentry case "I have no current entry".  The
error returns are usually an explicit "there was some problem during
the lookup".

James

Re: Potential static analysis ideas

[Linus Torvalds]

On Mon, Jul 26, 2021 at 10:55 AM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> Actually, I don't think so ... we have NULL return all over the inode
> and dentry code.  It's a legitimate return for "I couldn't find what
> you asked for" or in the dentry case "I have no current entry".  The
> error returns are usually an explicit "there was some problem during
> the lookup".

Absolutely. We have several cases of "valid pointer, NULL _or_ error pointer".

Some of those things are not always about error pointers either. An
example of this same kind of "three-way answer" is get_cached_acl(),
which can return

 - an actual honest-to-goodness real posix_acl pointer

 - NULL for "there is no ACL data"

 -  special value for "no cached data", need to ask the filesystem

That particular case isn't using the error pointers, but it's very
similar in nature to that "this is an explicit error" vs "don't know"
kind of thing.

Anyway, gcc has this completely ass-backwards "nonnull" function
attribute, sadly it's pure garbage. It's not a "type of this pointer"
thing, it's a "this function does not take NULL as argument X" thing.

We could relatively easily add a "__nonnull" type attribute  (and
"__errptr") and parse it in sparse, but it would be _all_ over the
place, and without real compiler support it's probably not worth it
(people already don't run sparse all that much, and a lot of the
things sparse can warn about are too verbose to be be used in
practice).

But in _theory_ it would be lovely to be able to say "this pointer
cannot be NULL" or "this pointer can be an error pointer", but I
suspect it really ends up being everywhere and more pain than gain.

                   Linus

Re: Potential static analysis ideas

[Laurent Pinchart]

Hi James,

On Mon, Jul 26, 2021 at 10:54:59AM -0700, James Bottomley wrote:
> On Mon, 2021-07-26 at 11:16 +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 26, 2021 at 11:08 AM Hannes Reinecke wrote:
> > > On 7/26/21 10:55 AM, Julia Lawall wrote:
> > > > On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> > > > > On Mon, Jul 26, 2021 at 9:53 AM Geert Uytterhoeven wrote:
> > > > > > On Mon, Jul 26, 2021 at 9:26 AM Arnd Bergmann wrote:
> > > > > > > On Sun, Jul 25, 2021 at 1:45 AM NeilBrown wrote:
> > > > > > > > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > > > > > > > > To make it work well, you need to know if frob() and/or the current
> > > > > > > > > > function return an error code or not.  While you can use some heuristics
> > > > > > > > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > > > > > > > indicate if a function returns an error code, or an error pointer?
> > > > > > > > > 
> > > > > > > > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > > > > > > > > 
> > > > > > > > > I think it would be useful, if not for the tools, at least for
> > > > > > > > > developers.
> > > > > > > > 
> > > > > > > > Agreed.  I added some code to smatch so that I could annotate pointers to
> > > > > > > > say if they are allowed to be NULL.  The implementation isn't perfect,
> > > > > > > > but I love having that extra documentation about when I do or don't have
> > > > > > > > to check for NULL.
> > > > > > > 
> > > > > > > I can think of four different annotations that limit what a pointer return from
> > > > > > > a function can be:
> > > > > > > 
> > > > > > > a) either a valid pointer or NULL, but never an error pointer,
> > > > > > > b) either a valid pointer or an error pointer, but not NULL,
> > > > > > > c) always a valid pointer, never NULL or an error,
> > > > > > > d) always NULL, but callers are expected to check for error
> > > > > > > pointers.
> > > > > > 
> > > > > > e) either a valid pointer, NULL, or an error pointer
> > > > > > 
> > > > > > The last pattern is seen with the various *get*_optional()
> > > > > > functions.
> > > > > 
> > > > > I would always consider those the exact bug that I meant with "because
> > > > > everyone gets those wrong". I think the idea of the "optional" functions is
> > > > > that you have two implementations b) and d) and pick one of them
> > > > > at compile time. To the caller this means either an error pointer or
> > > > > success, but checking for NULL is a bug in the caller, while conditionally
> > > > > returning NULL or ERR_PTR() would be a bug in the interface.
> > > > 
> > > > I'm not sure to understand the "bug in the caller" part.  Couldn't there
> > > > be two possible definitions of the called function, according to different
> > > > configuration options, and a single caller that calls both?
> > > > 
> > > > Also, over 230 files contain functions that return both NULL and ERR_PTR.
> > > > A random example, chosen for conciseness, is the following from
> > > > fs/overlayfs/inode.c:
> > > > 
> > > > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
> > > >                                 bool is_upper)
> > > > {
> > > >       struct inode *inode, *key = d_inode(real);
> > > > 
> > > >          inode = ilookup5(sb, (unsigned long) key, ovl_inode_test, key);
> > > >       if (!inode)
> > > >                  return NULL;
> > > > 
> > > >          if (!ovl_verify_inode(inode, is_upper ? NULL : real,
> > > >                                is_upper ? real : NULL, false)) {
> > > >                  iput(inode);
> > > >                  return ERR_PTR(-ESTALE);
> > > >          }
> > > > 
> > > >          return inode;
> > > > }
> > > > 
> > > And that I would consider a coding error.
> > > If a function is able to return an error pointer it should _always_
> > > return an error pointer; here it would be trivial to return -ENXIO
> > > instead of NULL in the first condition.
> > > 
> > > Not doing so is just sloppy programming IMO.
> > 
> > In this case I agree.
> 
> Actually, I don't think so ... we have NULL return all over the inode
> and dentry code.  It's a legitimate return for "I couldn't find what
> you asked for" or in the dentry case "I have no current entry".  The
> error returns are usually an explicit "there was some problem during
> the lookup".

It's a matter of semantics. From a technical point of view, both NULL or
a particular error code can mean "not found". What has been bothering me
for a long time, and keeps doing so, is the lack of standardization in
the semantics. Even within a subsystem, different semantics can be used,
and that's the source of bugs and overall pain for developers. I'm not
sure if complete standardization at the kernel level is possible, but
any step we can take in that direction would I believe be an
improvement. At the very least, we need a way for developers to easily
figure out what semantics a given function uses. Having to read the
source code, sometimes diving deep in call stacks, to find if a function
can return NULL, is too painful.

-- 
Regards,

Laurent Pinchart

Re: Potential static analysis ideas

[NeilBrown]

On Mon, 26 Jul 2021, Arnd Bergmann wrote:
> On Sun, Jul 25, 2021 at 1:45 AM NeilBrown <neilb@suse.de> wrote:
> > On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> > > >
> > > > To make it work well, you need to know if frob() and/or the current
> > > > function return an error code or not.  While you can use some heuristics
> > > > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > > > indicate if a function returns an error code, or an error pointer?
> > >
> > > https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> > >
> > > I think it would be useful, if not for the tools, at least for
> > > developers.
> >
> > Agreed.  I added some code to smatch so that I could annotate pointers to
> > say if they are allowed to be NULL.  The implementation isn't perfect,
> > but I love having that extra documentation about when I do or don't have
> > to check for NULL.
> 
> I can think of four different annotations that limit what a pointer return from
> a function can be:
> 
> a) either a valid pointer or NULL, but never an error pointer,
> b) either a valid pointer or an error pointer, but not NULL,
> c) always a valid pointer, never NULL or an error,
> d) always NULL, but callers are expected to check for error pointers.
> 
> The last one is the tricky bit because there are a couple of subsystems
> that intentionally return an opaque NULL pointer to indicate success
> when the subsystem is compile-time disabled, and expect callers to not
> dereference that pointer but instead pass it into other stubbed-out
> interfaces. I think this one is rather specific to the kernel, but also really
> useful to have in a static checker because everyone gets those wrong.

And now we see the worms wriggling their way out of the can....

Some pointers can also have the 'lsb' overloaded, sometimes as a
bit-spin-lock.
And pointers stored in a structure can have temporal type requirements -
different at various places in the life cycle.
Most obviously, "non-NULL" pointers can be NULL (or unitiallised) during
inititalization, and may have other inconsistencies during finalization.
But it isn't hard to find more complexity.

"context" structures are particularly suseptible to this.  I've been
looking at 'struct fs_context' recently.  Normally you would use one of
the allocation functions which will always set ->ops and a few other
fields. So maybe ->ops is "always valid".
Then you initialize some extra bits.  Maybe call vfs_parse_fs_string()
etc.
Finally you call vfs_create_mount().  But you *can* call
vfs_create_mount() after only setting ->root ->sb_flags and ->source.
->ops can be uninitialised!

So what annotations would you give to the various fields? To capture
this completely you would need a series of sub-types of 'fs_context',
each with different guarantees or requirements about the values of
different fields.

I love contemplating what the "perfect" system would be, but I doubt a
near-perfect system would be much fun to program in, and that last 20%
to get to perfection would take a while.

So if we wanted to do something like this, it would need to be
completely opt-in.  The default would be current behaviour, and we would
want to have annotations for both what is, and what isn't, promised.

"what is" would be something like "this pointer is never NULL".
"what isn't" would be more like __must_check.  "this pointer might be
NULL and you really have to check before you dereference it".
For an unannotated pointer, it is not an error to test if it is NULL, or
to dereference without testing.

And, of course, we would need to be able to cast between the different
promises.

NeilBrown

Re: Potential static analysis ideas

[NeilBrown]

On Tue, 27 Jul 2021, Linus Torvalds wrote:
> 
> Anyway, gcc has this completely ass-backwards "nonnull" function
> attribute, sadly it's pure garbage. It's not a "type of this pointer"
> thing, it's a "this function does not take NULL as argument X" thing.

Yeah, I found that annoying too.  It seems to be purely about
optimization.  Marking a function parameter as "nonnull" means the
compiler can optimize away any tests against NULL.

> 
> We could relatively easily add a "__nonnull" type attribute  (and
> "__errptr") and parse it in sparse, but it would be _all_ over the

sparse already parses __attribute__((safe)) which seems to mean
something like __nonnull.  It needs a bit of work to make it useful.

> place, and without real compiler support it's probably not worth it
> (people already don't run sparse all that much, and a lot of the
> things sparse can warn about are too verbose to be be used in
> practice).

I'm not convinced that "real compiler support" is essential.
checkpatch.pl does provide some value despite not being "real compiler
support" and not being mandated.

NeilBrown

Re: Potential static analysis ideas

[Dan Carpenter]

On Mon, Jul 26, 2021 at 08:50:39AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote:
> > Rust has many good static analysis features but if we wanted we could
> > implement a number of stricter rules in C.  With Smatch I have tried to
> > focus on exclusively on finding bugs because everyone can agree that
> > bugs are bad.  But if some subsystems wanted to implement stricter rules
> > just as a hardenning measure then that's a doable thing.
> > 
> > For example, I've tried a bunch of approaches to warning about when the
> > user can trigger an integer overflow.  The challenge is that most
> > integer overflows are harmless and do not cause a real life bug.
> 
> I would not want overflow checks for unsigned integers, but it might
> be helpful for signed integers.  But yes, most of us rely on fwrapv,
> so that kernelwide checks for signed integer overflow will be quite noisy.

Since we use -fwrapv then even signed integer overflows are defined and
I haven't seen a way that checking for signed integer overflows can be
useful.

With integer overflows I'm more talking about integer overflows from the
user.  And I imagine a subsystem specific thing as a kind of "We want
extra security but aren't ready to port everything to Rust" type option.

I have almost 2 thousand of these warnings.  This first example is from
the ioctl and probably root only.  Plus commit 6d13de1489b6 ("uaccess:
disallow > INT_MAX copy sizes") really improved security.

drivers/fpga/dfl-fme-pr.c
    83          if (copy_from_user(&port_pr, argp, minsz))
    84                  return -EFAULT;
    85  
    86          if (port_pr.argsz < minsz || port_pr.flags)
    87                  return -EINVAL;
    88  
    89          /* get fme header region */
    90          fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
    91                                                 FME_FEATURE_ID_HEADER);
    92  
    93          /* check port id */
    94          v = readq(fme_hdr + FME_HDR_CAP);
    95          if (port_pr.port_id >= FIELD_GET(FME_CAP_NUM_PORTS, v)) {
    96                  dev_dbg(&pdev->dev, "port number more than maximum\n");
    97                  return -EINVAL;
    98          }
    99  
   100          /*
   101           * align PR buffer per PR bandwidth, as HW ignores the extra padding
   102           * data automatically.
   103           */
   104          length = ALIGN(port_pr.buffer_size, 4);
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This ALIGN() operation can overflow but only to zero.

   105  
   106          buf = vmalloc(length);

kmalloc(() allows zero size allocations but vmalloc() will return NULL.
And actually, in April, Nicholas Piggin made it trigger a WARN_ONCE().

   107          if (!buf)
   108                  return -ENOMEM;
   109  
   110          if (copy_from_user(buf,
   111                             (void __user *)(unsigned long)port_pr.buffer_address,
   112                             port_pr.buffer_size)) {
                                   ^^^^^^^^^^^^^^^^^^^
So this can't corrupt memory for the reasons given above.

(It's still buggy because it doesn't zero out the last three bytes
between port_pr.buffer_size and length, but that's a different issue).

regards,
dan carpenter

Re: Potential static analysis ideas

[Geert Uytterhoeven]

On Tue, Jul 27, 2021 at 11:38 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Jul 26, 2021 at 08:50:39AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote:
> > > Rust has many good static analysis features but if we wanted we could
> > > implement a number of stricter rules in C.  With Smatch I have tried to
> > > focus on exclusively on finding bugs because everyone can agree that
> > > bugs are bad.  But if some subsystems wanted to implement stricter rules
> > > just as a hardenning measure then that's a doable thing.
> > >
> > > For example, I've tried a bunch of approaches to warning about when the
> > > user can trigger an integer overflow.  The challenge is that most
> > > integer overflows are harmless and do not cause a real life bug.
> >
> > I would not want overflow checks for unsigned integers, but it might
> > be helpful for signed integers.  But yes, most of us rely on fwrapv,
> > so that kernelwide checks for signed integer overflow will be quite noisy.
>
> Since we use -fwrapv then even signed integer overflows are defined and
> I haven't seen a way that checking for signed integer overflows can be
> useful.

For all people using git grep and investigating: we don't pass -fwrapv
explicitly, it is implied by -fno-strict-overflow.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Potential static analysis ideas

[Paul E. McKenney]

On Tue, Jul 27, 2021 at 12:38:08PM +0300, Dan Carpenter wrote:
> On Mon, Jul 26, 2021 at 08:50:39AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 23, 2021 at 10:10:23PM +0300, Dan Carpenter wrote:
> > > Rust has many good static analysis features but if we wanted we could
> > > implement a number of stricter rules in C.  With Smatch I have tried to
> > > focus on exclusively on finding bugs because everyone can agree that
> > > bugs are bad.  But if some subsystems wanted to implement stricter rules
> > > just as a hardenning measure then that's a doable thing.
> > > 
> > > For example, I've tried a bunch of approaches to warning about when the
> > > user can trigger an integer overflow.  The challenge is that most
> > > integer overflows are harmless and do not cause a real life bug.
> > 
> > I would not want overflow checks for unsigned integers, but it might
> > be helpful for signed integers.  But yes, most of us rely on fwrapv,
> > so that kernelwide checks for signed integer overflow will be quite noisy.
> 
> Since we use -fwrapv then even signed integer overflows are defined and
> I haven't seen a way that checking for signed integer overflows can be
> useful.

Just because the compiler defines something does not mean that it cannot
be involved in a bug.  ;-)

> With integer overflows I'm more talking about integer overflows from the
> user.  And I imagine a subsystem specific thing as a kind of "We want
> extra security but aren't ready to port everything to Rust" type option.

Which was what I was also imagining, but along different lines.

But I agree that what you are proposing might be useful.

							Thanx, Paul

> I have almost 2 thousand of these warnings.  This first example is from
> the ioctl and probably root only.  Plus commit 6d13de1489b6 ("uaccess:
> disallow > INT_MAX copy sizes") really improved security.
> 
> drivers/fpga/dfl-fme-pr.c
>     83          if (copy_from_user(&port_pr, argp, minsz))
>     84                  return -EFAULT;
>     85  
>     86          if (port_pr.argsz < minsz || port_pr.flags)
>     87                  return -EINVAL;
>     88  
>     89          /* get fme header region */
>     90          fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
>     91                                                 FME_FEATURE_ID_HEADER);
>     92  
>     93          /* check port id */
>     94          v = readq(fme_hdr + FME_HDR_CAP);
>     95          if (port_pr.port_id >= FIELD_GET(FME_CAP_NUM_PORTS, v)) {
>     96                  dev_dbg(&pdev->dev, "port number more than maximum\n");
>     97                  return -EINVAL;
>     98          }
>     99  
>    100          /*
>    101           * align PR buffer per PR bandwidth, as HW ignores the extra padding
>    102           * data automatically.
>    103           */
>    104          length = ALIGN(port_pr.buffer_size, 4);
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This ALIGN() operation can overflow but only to zero.
> 
>    105  
>    106          buf = vmalloc(length);
> 
> kmalloc(() allows zero size allocations but vmalloc() will return NULL.
> And actually, in April, Nicholas Piggin made it trigger a WARN_ONCE().
> 
>    107          if (!buf)
>    108                  return -ENOMEM;
>    109  
>    110          if (copy_from_user(buf,
>    111                             (void __user *)(unsigned long)port_pr.buffer_address,
>    112                             port_pr.buffer_size)) {
>                                    ^^^^^^^^^^^^^^^^^^^
> So this can't corrupt memory for the reasons given above.
> 
> (It's still buggy because it doesn't zero out the last three bytes
> between port_pr.buffer_size and length, but that's a different issue).
> 
> regards,
> dan carpenter