Re: Potential static analysis ideas

["Paul E. McKenney" <paulmck@kernel.org>]

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