Re: Potential static analysis ideas

[Dan Carpenter <dan.carpenter@oracle.com>]

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