[bug report] fsi: Add cfam char devices

[Benjamin Herrenschmidt]

On Tue, 2023-10-10 at 16:35 +0300, Dan Carpenter wrote:
> Hello Benjamin Herrenschmidt,
> 
> The patch d1dcd6782576: "fsi: Add cfam char devices" from Jun 21,
> 2018, leads to the following Smatch static checker warning:
> 
>         drivers/fsi/fsi-core.c:717 cfam_write()
>         error: check that 'write_len' is capped
> 
> drivers/fsi/fsi-core.c
>     696 static ssize_t cfam_write(struct file *filep, const char __user *buf,
>     697                           size_t count, loff_t *offset)
>     698 {
>     699         struct fsi_slave *slave = filep->private_data;
>     700         size_t total_len, write_len;
>     701         loff_t off = *offset;
>     702         ssize_t rc;
>     703 
>     704 
>     705         if (off < 0)
>     706                 return -EINVAL;
>     707 
>     708         if (off > 0xffffffff || count > 0xffffffff || off + count > 0xffffffff)
>     709                 return -EINVAL;
>     710 
>     711         for (total_len = 0; total_len < count; total_len += write_len) {
>     712                 __be32 data;
>     713 
>     714                 write_len = min_t(size_t, count, 4);
>     715                 write_len -= off & 0x3;
> 
> This offset calculation is strange.  Assume we are writing 2 bytes to
> offset 3.  That means that write_len is set to 2 - 3 which is -1.

Right, that's kind of bogus. From my vague memories (I've long moved on
and don't work for IBM anymore), the goal here was to have a limit of
writing 2 bytes if the offset is 2 bytes into the word, and 1 byte of
it's 3 byte into the word (etc...).

But the test didn't take into account an attempt to write an unaligned
quantity that is smaller than the word. Ouch.

It should probably be

           write_len = min_t(size_t, count, 4 - (off & 3));

Don't you think ?

I'll let Eddie (or others) handle this ...

Cheers,
Ben.


>     716 
> --> 717                 rc = copy_from_user(&data, buf + total_len, write_len);
> 
> In olden days that would have been a really easy to exploit buffer
> overflow but now copy_from_user() will just trigger a WARN_ON() for
> negatives.
> 
>     718                 if (rc) {
>     719                         rc = -EFAULT;
>     720                         goto fail;
>     721                 }
>     722                 rc = fsi_slave_write(slave, off, &data, write_len);
>     723                 if (rc)
>     724                         goto fail;
>     725                 off += write_len;
>     726         }
>     727         rc = count;
>     728  fail:
>     729         *offset = off;
>     730         return rc;
>     731 }
> 
> regards,
> dan carpenter