Date: Sat, 25 May 2024 23:48:28 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Jiasheng Jiang <jiashengjiangcool@...look.com>
Cc: brauner@...nel.org, jack@...e.cz, arnd@...db.de, gregkh@...e.de,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] libfs: fix implicitly cast in simple_attr_write_xsigned()

On Sat, May 25, 2024 at 07:55:52PM +0000, Jiasheng Jiang wrote:
> > On Wed, May 15, 2024 at 03:17:25PM +0000, Jiasheng Jiang wrote:
> >> Return 0 to indicate failure and return "len" to indicate success.
> >> It was hard to distinguish success or failure if "len" equals the error
> >> code after the implicit cast.
> >> Moreover, eliminating implicit cast is a better practice.
> > 
> > According to whom?
> > 
> 
> Programmers can easily overlook implicit casts, leading to unknown
> behavior (e.g., this bug).

Which bug is "this" in the above refering to?

> Converting implicit casts to explicit casts can help prevent future
> errors.
> 
> > Merits of your ex cathedra claims aside, you do realize that functions
> > have calling conventions because they are, well, called, right?
> > And changing the value returned in such and such case should be
> > accompanied with the corresponding change in the _callers_.
> > 
> > Al, wondering if somebody had decided to play with LLM...
> 
> As the comment shows that "ret = len; /* on success, claim we got the
> whole input */", the return value should be checked to determine whether
> it equals "len".
> 
> Moreover, if "len" is 0, the previous copy_from_user() will fail and
> return an error.
> Therefore, 0 is an illegal value for "len". Besides, in the linux kernel,
> all the callers of simple_attr_write_xsigned() return the return value of
> simple_attr_write_xsigned().

Lovely.  "Callers are expected to check somewhere; immediate callers just
return it as-is to their callers, therefore we are done".  So where would
those checks be?  Deeper in the call chain?  Let's look at the call chains,
then...

; git grep -n -w simple_attr_write_xsigned
fs/libfs.c:1341:static ssize_t simple_attr_write_xsigned(struct file *file, const char __user *buf,
fs/libfs.c:1380:        return simple_attr_write_xsigned(file, buf, len, ppos, false);
fs/libfs.c:1387:        return simple_attr_write_xsigned(file, buf, len, ppos, true);
;

Two callers, one of them being
ssize_t simple_attr_write(struct file *file, const char __user *buf,
                          size_t len, loff_t *ppos)
{
	return simple_attr_write_xsigned(file, buf, len, ppos, false);
}

and another
ssize_t simple_attr_write_signed(struct file *file, const char __user *buf,
                          size_t len, loff_t *ppos)
{
	return simple_attr_write_xsigned(file, buf, len, ppos, true);
}

All right, who calls those?

; git grep -n -w simple_attr_write
arch/powerpc/platforms/cell/spufs/file.c:455:   .write = simple_attr_write,
drivers/gpu/drm/imagination/pvr_params.c:123:           .write = simple_attr_write,                \
fs/debugfs/file.c:485:          ret = simple_attr_write(file, buf, len, ppos);
fs/libfs.c:1377:ssize_t simple_attr_write(struct file *file, const char __user *buf,
fs/libfs.c:1382:EXPORT_SYMBOL_GPL(simple_attr_write);
include/linux/fs.h:3501:        .write   = (__is_signed) ? simple_attr_write_signed : simple_attr_write,        \
include/linux/fs.h:3523:ssize_t simple_attr_write(struct file *file, const char __user *buf,
virt/kvm/kvm_main.c:6117:       .write = simple_attr_write,
;

In addition to one direct caller it is used as ->write method instances.
What in?

static const struct file_operations spufs_cntl_fops = {
        .open = spufs_cntl_open,
        .release = spufs_cntl_release,
        .read = simple_attr_read,
        .write = simple_attr_write,
        .llseek = no_llseek,
        .mmap = spufs_cntl_mmap,
};

static struct {
#define X(type_, name_, value_, desc_, mode_, update_) \
        const struct file_operations name_;
        PVR_DEVICE_PARAMS
#undef X
} pvr_device_param_debugfs_fops = {
#define X(type_, name_, value_, desc_, mode_, update_)     \
        .name_ = {                                         \
                .owner = THIS_MODULE,                      \
                .open = __pvr_device_param_##name_##_open, \
                .release = simple_attr_release,            \
                .read = simple_attr_read,                  \
                .write = simple_attr_write,                \
                .llseek = generic_file_llseek,             \
        },   
        PVR_DEVICE_PARAMS
#undef X
};

static const struct file_operations __fops = {                          \
        .owner   = THIS_MODULE,                                         \
        .open    = __fops ## _open,                                     \
        .release = simple_attr_release,                                 \
        .read    = simple_attr_read,                                    \
        .write   = (__is_signed) ? simple_attr_write_signed : simple_attr_write,        \
        .llseek  = generic_file_llseek,                                 \
}

static const struct file_operations stat_fops_per_vm = {
        .owner = THIS_MODULE,
        .open = kvm_stat_data_open,
        .release = kvm_debugfs_release,
        .read = simple_attr_read,   
        .write = simple_attr_write,
        .llseek = no_llseek,
};

So all of those are file_operations::write instances.  The caller?

static ssize_t debugfs_attr_write_xsigned(struct file *file, const char __user *buf,
                         size_t len, loff_t *ppos, bool is_signed)
{
        struct dentry *dentry = F_DENTRY(file);
        ssize_t ret;

        ret = debugfs_file_get(dentry);
        if (unlikely(ret))
                return ret;
        if (is_signed)
                ret = simple_attr_write_signed(file, buf, len, ppos);
        else
                ret = simple_attr_write(file, buf, len, ppos);
        debugfs_file_put(dentry);
        return ret;
}

itself called in

ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
                         size_t len, loff_t *ppos)
{
        return debugfs_attr_write_xsigned(file, buf, len, ppos, false);
}

and

ssize_t debugfs_attr_write_signed(struct file *file, const char __user *buf,
                         size_t len, loff_t *ppos)
{
        return debugfs_attr_write_xsigned(file, buf, len, ppos, true);
}

Both of those are used only in
static const struct file_operations __fops = {                          \
        .owner   = THIS_MODULE,                                         \
        .open    = __fops ## _open,                                     \
        .release = simple_attr_release,                                 \
        .read    = debugfs_attr_read,                                   \
        .write   = (__is_signed) ? debugfs_attr_write_signed : debugfs_attr_write,      \
        .llseek  = no_llseek,                                           \
}
in expansion of DEFINE_DEBUGFS_ATTRIBUTE_XSIGNED().


In other words, all call chains go through some ->write() method call,
with return value ending up that of ->write() instance.

Now, looking for the places where file_operations ->write() can be called
is a bit more tedious, but one would expect to see at least some near write(2).
Which lives in fs/read_write.c, where we see this:

ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_t *pos)
{
        ssize_t ret;

        if (!(file->f_mode & FMODE_WRITE))
                return -EBADF;
        if (!(file->f_mode & FMODE_CAN_WRITE))
                return -EINVAL;
        if (unlikely(!access_ok(buf, count)))
                return -EFAULT;

        ret = rw_verify_area(WRITE, file, pos, count);
        if (ret)
                return ret;
        if (count > MAX_RW_COUNT)
                count =  MAX_RW_COUNT;
        file_start_write(file);
        if (file->f_op->write)
                ret = file->f_op->write(file, buf, count, pos);
        else if (file->f_op->write_iter)
                ret = new_sync_write(file, buf, count, pos);
        else   
                ret = -EINVAL;
        if (ret > 0) {
                fsnotify_modify(file);
                add_wchar(current, ret);
        }
        inc_syscw(current);
        file_end_write(file);
        return ret;
}
and from there it's very close to write(2):

ssize_t ksys_write(unsigned int fd, const char __user *buf, size_t count)
{
        struct fd f = fdget_pos(fd);
        ssize_t ret = -EBADF;

        if (f.file) {
                loff_t pos, *ppos = file_ppos(f.file);
                if (ppos) {
                        pos = *ppos;
                        ppos = &pos;
                }
                ret = vfs_write(f.file, buf, count, ppos);
                if (ret >= 0 && ppos)
                        f.file->f_pos = pos;
                fdput_pos(f);
        }

        return ret;
}

and finally
SYSCALL_DEFINE3(write, unsigned int, fd, const char __user *, buf,
                size_t, count)
{
        return ksys_write(fd, buf, count);
}

So here's at least one call chain where return value is propagated
all the way back to userland, without *ANY* alleged comparisons with
the length argument (which, again, comes directly from write(2) in
this particular callchain).

So before your change write(2) called with arguments that stepped onto
that returned -EACCES; with that change on the same arguments it returns
0.

In libc that translates into "return -1, set errno to EACCES" and "return 0"
respectively.

I hope the above is sufficient to explain the problem with the reasoning in
your patch.

_ANYTHING_ that changes calling conventions of a function must either
verify that all callers are fine with the change, or adjust them accordingly.
If your change alters the calling conventions of those callers, the same
applies to them, etc.

What's more, your change is very clearly losing information - the current
calling conventions for ->write() are "return the number of bytes written
(possibly less than demanded) or, in case of error with no bytes written,
a small negative number representing that error (negated errno.h constants)"
and with your change you get no way to report _which_ error has occured.
You can't adjust for that at any point of call chain - and pretty soon
you run into the userland boundary anyway, at which point the calling
conventions are cast in stone.

Please note that reading comments does _not_ replace checking what's
really going on, especially when the comment is vague enough to be
misinterpreted.  It doesn't say that callers will check that return value
will be compared to len argument - it says that on success this instance
of ->write() will claim to have consumed the entire buffer passed to it.
Further look into how it parses the input shows that it will e.g. treat
"12" and "12\n" identically, reporting 2 and 3 bytes resp. having been
written, even though the final newline is ignored in the latter case.
In other words, it claims (correctly) that it won't result in short
writes.  It does not promise anything about the checks to be made
by callers.

NAK.

Powered by blists - more mailing lists