Re: git: 0fce2dc15730 - main - LinuxKPI,lindebugfs: add u8 base type and blob support

On 11/28/22 9:24 AM, Bjoern A. Zeeb wrote:
> The branch main has been updated by bz:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=0fce2dc1573019d0732f33fa7c26cc228655d3e8
> 
> commit 0fce2dc1573019d0732f33fa7c26cc228655d3e8
> Author:     Bjoern A. Zeeb <bz@FreeBSD.org>
> AuthorDate: 2022-10-22 18:12:16 +0000
> Commit:     Bjoern A. Zeeb <bz@FreeBSD.org>
> CommitDate: 2022-11-28 17:21:50 +0000
> 
>      LinuxKPI,lindebugfs: add u8 base type and blob support
>      
>      Add debugfs_create_u8() based on other already present implementations.
>      Add a read-only implementation for debugfs_create_blob().
>      
>      Both are needed for iwlwifi debugfs support.

This passes kernel pointers to copyout because debugfs is kind of broken due to
quirks in how linux file operations are implemented.
       
>      Sponsored by:   The FreeBSD Foundation
>      MFC after:      3 days
>      OKed by:        jfree (earlier version)
>      Differential Revision: https://reviews.freebsd.org/D37090
> ---
>   sys/compat/lindebugfs/lindebugfs.c                 | 66 ++++++++++++++++++++++
>   sys/compat/linuxkpi/common/include/linux/debugfs.h | 11 +++-
>   2 files changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/sys/compat/lindebugfs/lindebugfs.c b/sys/compat/lindebugfs/lindebugfs.c
> index b88caf9c3140..b72ceb5e0be9 100644
> --- a/sys/compat/lindebugfs/lindebugfs.c
> +++ b/sys/compat/lindebugfs/lindebugfs.c
> @@ -394,6 +423,43 @@ debugfs_create_ulong(const char *name, umode_t mode, struct dentry *parent, unsi
>   	    &fops_ulong_ro, &fops_ulong_wo);
>   }
>   
> +
> +static ssize_t
> +fops_blob_read(struct file *filp, char __user *ubuf, size_t read_size, loff_t *ppos)

This is not actually a __user pointer.  Note that simple_attr_read/write use plain pointers.
The only invocations of this function are in debugfs_fill:

	rc = -ENODEV;
	if (uio->uio_rw == UIO_READ && d->dm_fops->read) {
		rc = -ENOMEM;
		buf = (char *) malloc(sb->s_size, M_DFSINT, M_ZERO | M_NOWAIT);
		if (buf != NULL) {
			rc = d->dm_fops->read(&lf, buf, sb->s_size, &off);
			if (rc > 0)
				sbuf_bcpy(sb, buf, strlen(buf));

			free(buf, M_DFSINT);
		}
	} else if (uio->uio_rw == UIO_WRITE && d->dm_fops->write) {
		sbuf_finish(sb);
		rc = d->dm_fops->write(&lf, sbuf_data(sb), sbuf_len(sb), &off);
	}

> +{
> +	struct debugfs_blob_wrapper *blob;
> +
> +	blob = filp->private_data;
> +	if (blob == NULL)
> +		return (-EINVAL);
> +	if (blob->size == 0 || blob->data == NULL)
> +		return (-EINVAL);
> +
> +	return (simple_read_from_buffer(ubuf, read_size, ppos, blob->data, blob->size));
> +}

This means you can't use simple_read_from_buffer() here as it is a wrapper around
copyout.  At least some architectures fail copyout calls where the user pointer is
a kernel pointer (e.g. arm64 and RISC-V).  I haven't looked if amd64 has the
misfeature of letting it work.  I'll have to fix this for CheriBSD downstream, but
I'll try to post a review using an inlined version of simple_read_from_buffer that
just uses memcpy directly.

-- 
John Baldwin