[PATCHES] uaccess simple access_ok() removals

[PATCHES] uaccess simple access_ok() removals

[Al Viro]

	One of the uaccess-related branches; this one is just the
cases when access_ok() calls are trivially pointless - the address
in question gets fed only to primitives that do access_ok() checks
themselves.  This stuff sits in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #uaccess.access_ok
and it's on top of #fixes (which is already merged into mainline on
Apr 28).
	Individual patches in followups; if nobody screams - into #for-next
it goes...

	There are other uaccess-related branches; I'll be posting them
for review over the next few days.  My apologies if this one comes with
a buggered Cc - this is the first time I have to deal with a series with
Cc lists varying that much; I hope I manage to get git-send-email do
the right thing, but...

Shortlog:
Al Viro (20):
      dlmfs_file_write(): get rid of pointless access_ok()
      fat_dir_ioctl(): hadn't needed that access_ok() for more than a decade...
      btrfs_ioctl_send(): don't bother with access_ok()
      FIEMAP: don't bother with access_ok()
      tomoyo_write_control(): get rid of pointless access_ok()
      n_hdlc_tty_read(): remove pointless access_ok()
      nvram: drop useless access_ok()
      cm4000_cs.c cmm_ioctl(): get rid of pointless access_ok()
      drivers/fpga/dfl-fme-pr.c: get rid of pointless access_ok()
      drivers/fpga/dfl-afu-dma-region.c: get rid of pointless access_ok()
      amifb: get rid of pointless access_ok() calls
      omapfb: get rid of pointless access_ok() calls
      drivers/crypto/ccp/sev-dev.c: get rid of pointless access_ok()
      via-pmu: don't bother with access_ok()
      drm_read(): get rid of pointless access_ok()
      efi_test: get rid of pointless access_ok()
      lpfc_debugfs: get rid of pointless access_ok()
      usb: get rid of pointless access_ok() calls
      hfi1: get rid of pointless access_ok()
      vmci_host: get rid of pointless access_ok()
Diffstat:
 drivers/char/nvram.c                            |  4 ----
 drivers/char/pcmcia/cm4000_cs.c                 | 14 --------------
 drivers/crypto/ccp/sev-dev.c                    | 15 +++------------
 drivers/firmware/efi/test/efi_test.c            | 12 ------------
 drivers/fpga/dfl-afu-dma-region.c               |  4 ----
 drivers/fpga/dfl-fme-pr.c                       |  4 ----
 drivers/gpu/drm/drm_file.c                      |  3 ---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c       |  7 -------
 drivers/macintosh/via-pmu.c                     |  2 --
 drivers/misc/vmw_vmci/vmci_host.c               |  2 --
 drivers/scsi/lpfc/lpfc_debugfs.c                | 12 ------------
 drivers/tty/n_hdlc.c                            |  7 -------
 drivers/usb/core/devices.c                      |  2 --
 drivers/usb/core/devio.c                        |  9 ---------
 drivers/usb/gadget/function/f_hid.c             |  6 ------
 drivers/video/fbdev/amifb.c                     |  4 ----
 drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c |  3 ---
 fs/btrfs/send.c                                 |  7 -------
 fs/ext4/ioctl.c                                 |  5 -----
 fs/fat/dir.c                                    |  4 ----
 fs/ioctl.c                                      |  5 -----
 fs/ocfs2/dlmfs/dlmfs.c                          |  3 ---
 security/tomoyo/common.c                        |  2 --
 23 files changed, 3 insertions(+), 133 deletions(-)

[PATCH 01/20] dlmfs_file_write(): get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

address passed only to copy_from_user()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ocfs2/dlmfs/dlmfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
index 1de77f1a600b..a06f19b67d3b 100644
--- a/fs/ocfs2/dlmfs/dlmfs.c
+++ b/fs/ocfs2/dlmfs/dlmfs.c
@@ -291,9 +291,6 @@ static ssize_t dlmfs_file_write(struct file *filp,
 	if (!count)
 		return 0;
 
-	if (!access_ok(buf, count))
-		return -EFAULT;
-
 	lvb_buf = kmalloc(count, GFP_NOFS);
 	if (!lvb_buf)
 		return -ENOMEM;
-- 
2.11.0

[PATCH 02/20] fat_dir_ioctl(): hadn't needed that access_ok() for more than a decade...

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

address is passed only to put_user() and copy_to_user()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/fat/dir.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 054acd9fd033..b4ddf48fa444 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -804,8 +804,6 @@ static long fat_dir_ioctl(struct file *filp, unsigned int cmd,
 		return fat_generic_ioctl(filp, cmd, arg);
 	}
 
-	if (!access_ok(d1, sizeof(struct __fat_dirent[2])))
-		return -EFAULT;
 	/*
 	 * Yes, we don't need this put_user() absolutely. However old
 	 * code didn't return the right value. So, app use this value,
@@ -844,8 +842,6 @@ static long fat_compat_dir_ioctl(struct file *filp, unsigned cmd,
 		return fat_generic_ioctl(filp, cmd, (unsigned long)arg);
 	}
 
-	if (!access_ok(d1, sizeof(struct compat_dirent[2])))
-		return -EFAULT;
 	/*
 	 * Yes, we don't need this put_user() absolutely. However old
 	 * code didn't return the right value. So, app use this value,
-- 
2.11.0

[PATCH 03/20] btrfs_ioctl_send(): don't bother with access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

we do copy_from_user() on that range anyway

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/btrfs/send.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index c5f41bd86765..6a92ecf9eaa2 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7065,13 +7065,6 @@ long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg)
 		goto out;
 	}
 
-	if (!access_ok(arg->clone_sources,
-			sizeof(*arg->clone_sources) *
-			arg->clone_sources_count)) {
-		ret = -EFAULT;
-		goto out;
-	}
-
 	if (arg->flags & ~BTRFS_SEND_FLAG_MASK) {
 		ret = -EINVAL;
 		goto out;
-- 
2.11.0

[PATCH 04/20] FIEMAP: don't bother with access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

we use copy_to_user() on that thing anyway (and always had).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ext4/ioctl.c | 5 -----
 fs/ioctl.c      | 5 -----
 2 files changed, 10 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index bfc1281fc4cb..a0afd0338722 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -784,11 +784,6 @@ static int ext4_ioctl_get_es_cache(struct file *filp, unsigned long arg)
 	fieinfo.fi_extents_max = fiemap.fm_extent_count;
 	fieinfo.fi_extents_start = ufiemap->fm_extents;
 
-	if (fiemap.fm_extent_count != 0 &&
-	    !access_ok(fieinfo.fi_extents_start,
-		       fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
-		return -EFAULT;
-
 	if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
 		filemap_write_and_wait(inode->i_mapping);
 
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 282d45be6f45..f8181e2c2168 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -215,11 +215,6 @@ static int ioctl_fiemap(struct file *filp, struct fiemap __user *ufiemap)
 	fieinfo.fi_extents_max = fiemap.fm_extent_count;
 	fieinfo.fi_extents_start = ufiemap->fm_extents;
 
-	if (fiemap.fm_extent_count != 0 &&
-	    !access_ok(fieinfo.fi_extents_start,
-		       fieinfo.fi_extents_max * sizeof(struct fiemap_extent)))
-		return -EFAULT;
-
 	if (fieinfo.fi_flags & FIEMAP_FLAG_SYNC)
 		filemap_write_and_wait(inode->i_mapping);
 
-- 
2.11.0

[PATCH 05/20] tomoyo_write_control(): get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

address is passed only to get_user()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/tomoyo/common.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 1b467381986f..f93f8acd05f7 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -2662,8 +2662,6 @@ ssize_t tomoyo_write_control(struct tomoyo_io_buffer *head,
 
 	if (!head->write)
 		return -EINVAL;
-	if (!access_ok(buffer, buffer_len))
-		return -EFAULT;
 	if (mutex_lock_interruptible(&head->io_sem))
 		return -EINTR;
 	head->read_user_buf_avail = 0;
-- 
2.11.0

[PATCH 06/20] n_hdlc_tty_read(): remove pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

only copy_to_user() is done to the address in question

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/tty/n_hdlc.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 991f49ee4026..b09eac4b6d64 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -423,13 +423,6 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 	struct n_hdlc_buf *rbuf;
 	DECLARE_WAITQUEUE(wait, current);
 
-	/* verify user access to buffer */
-	if (!access_ok(buf, nr)) {
-		pr_warn("%s(%d) %s() can't verify user buffer\n",
-				__FILE__, __LINE__, __func__);
-		return -EFAULT;
-	}
-
 	add_wait_queue(&tty->read_wait, &wait);
 
 	for (;;) {
-- 
2.11.0

[PATCH 07/20] nvram: drop useless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

we are using copy_to_user()/memdup_user() anyway

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/char/nvram.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/char/nvram.c b/drivers/char/nvram.c
index 4667844eee69..8206412d25ba 100644
--- a/drivers/char/nvram.c
+++ b/drivers/char/nvram.c
@@ -232,8 +232,6 @@ static ssize_t nvram_misc_read(struct file *file, char __user *buf,
 	ssize_t ret;
 
 
-	if (!access_ok(buf, count))
-		return -EFAULT;
 	if (*ppos >= nvram_size)
 		return 0;
 
@@ -264,8 +262,6 @@ static ssize_t nvram_misc_write(struct file *file, const char __user *buf,
 	char *tmp;
 	ssize_t ret;
 
-	if (!access_ok(buf, count))
-		return -EFAULT;
 	if (*ppos >= nvram_size)
 		return 0;
 
-- 
2.11.0

[PATCH 08/20] cm4000_cs.c cmm_ioctl(): get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

copy_to_user()/copy_from_user() for everything

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/char/pcmcia/cm4000_cs.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 4edb4174a1e2..89681f07bc78 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -1404,7 +1404,6 @@ static long cmm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	unsigned int iobase = dev->p_dev->resource[0]->start;
 	struct inode *inode = file_inode(filp);
 	struct pcmcia_device *link;
-	int size;
 	int rc;
 	void __user *argp = (void __user *)arg;
 #ifdef CM4000_DEBUG
@@ -1441,19 +1440,6 @@ static long cmm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		DEBUGP(4, dev, "iocnr mismatch\n");
 		goto out;
 	}
-	size = _IOC_SIZE(cmd);
-	rc = -EFAULT;
-	DEBUGP(4, dev, "iocdir=%.4x iocr=%.4x iocw=%.4x iocsize=%d cmd=%.4x\n",
-	      _IOC_DIR(cmd), _IOC_READ, _IOC_WRITE, size, cmd);
-
-	if (_IOC_DIR(cmd) & _IOC_READ) {
-		if (!access_ok(argp, size))
-			goto out;
-	}
-	if (_IOC_DIR(cmd) & _IOC_WRITE) {
-		if (!access_ok(argp, size))
-			goto out;
-	}
 	rc = 0;
 
 	switch (cmd) {
-- 
2.11.0

[PATCH 09/20] drivers/fpga/dfl-fme-pr.c: get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

followed by copy_from_user()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/fpga/dfl-fme-pr.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/fpga/dfl-fme-pr.c b/drivers/fpga/dfl-fme-pr.c
index a233a53db708..1194c0e850e0 100644
--- a/drivers/fpga/dfl-fme-pr.c
+++ b/drivers/fpga/dfl-fme-pr.c
@@ -97,10 +97,6 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
 		return -EINVAL;
 	}
 
-	if (!access_ok((void __user *)(unsigned long)port_pr.buffer_address,
-		       port_pr.buffer_size))
-		return -EFAULT;
-
 	/*
 	 * align PR buffer per PR bandwidth, as HW ignores the extra padding
 	 * data automatically.
-- 
2.11.0

[PATCH 10/20] drivers/fpga/dfl-afu-dma-region.c: get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

Address is passed to get_user_pages_fast(), which does access_ok().
NB: this is called only from ->ioctl(), and only under USER_DS.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/fpga/dfl-afu-dma-region.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/fpga/dfl-afu-dma-region.c b/drivers/fpga/dfl-afu-dma-region.c
index 62f924489db5..d902acb36d14 100644
--- a/drivers/fpga/dfl-afu-dma-region.c
+++ b/drivers/fpga/dfl-afu-dma-region.c
@@ -324,10 +324,6 @@ int afu_dma_map_region(struct dfl_feature_platform_data *pdata,
 	if (user_addr + length < user_addr)
 		return -EINVAL;
 
-	if (!access_ok((void __user *)(unsigned long)user_addr,
-		       length))
-		return -EINVAL;
-
 	region = kzalloc(sizeof(*region), GFP_KERNEL);
 	if (!region)
 		return -ENOMEM;
-- 
2.11.0

[PATCH 11/20] amifb: get rid of pointless access_ok() calls

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

addresses passed only to get_user() and put_user()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/video/fbdev/amifb.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c
index 20e03e00b66d..6062104f3afb 100644
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -1855,8 +1855,6 @@ static int ami_get_var_cursorinfo(struct fb_var_cursorinfo *var,
 	var->yspot = par->crsr.spot_y;
 	if (size > var->height * var->width)
 		return -ENAMETOOLONG;
-	if (!access_ok(data, size))
-		return -EFAULT;
 	delta = 1 << par->crsr.fmode;
 	lspr = lofsprite + (delta << 1);
 	if (par->bplcon0 & BPC0_LACE)
@@ -1935,8 +1933,6 @@ static int ami_set_var_cursorinfo(struct fb_var_cursorinfo *var,
 		return -EINVAL;
 	if (!var->height)
 		return -EINVAL;
-	if (!access_ok(data, var->width * var->height))
-		return -EFAULT;
 	delta = 1 << fmode;
 	lofsprite = shfsprite = (u_short *)spritememory;
 	lspr = lofsprite + (delta << 1);
-- 
2.11.0

[PATCH 12/20] omapfb: get rid of pointless access_ok() calls

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

address is passed only to copy_to_user()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c b/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c
index 56995f44e76d..f40be68d5aac 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c
@@ -482,9 +482,6 @@ static int omapfb_memory_read(struct fb_info *fbi,
 	if (!display || !display->driver->memory_read)
 		return -ENOENT;
 
-	if (!access_ok(mr->buffer, mr->buffer_size))
-		return -EFAULT;
-
 	if (mr->w > 4096 || mr->h > 4096)
 		return -EINVAL;
 
-- 
2.11.0

[PATCH 13/20] drivers/crypto/ccp/sev-dev.c: get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

Contrary to the comments, those do *NOT* verify anything about
writability of memory, etc.

In all cases addresses are passed only to copy_to_user().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/crypto/ccp/sev-dev.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 896f190b9a50..7f97164cbafb 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -371,8 +371,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 		goto cmd;
 
 	/* allocate a physically contiguous buffer to store the CSR blob */
-	if (!access_ok(input.address, input.length) ||
-	    input.length > SEV_FW_BLOB_MAX_SIZE) {
+	if (input.length > SEV_FW_BLOB_MAX_SIZE) {
 		ret = -EFAULT;
 		goto e_free;
 	}
@@ -609,12 +608,6 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 
-	/* Check if we have write access to the userspace buffer */
-	if (input.address &&
-	    input.length &&
-	    !access_ok(input.address, input.length))
-		return -EFAULT;
-
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -730,15 +723,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 		goto cmd;
 
 	/* Allocate a physically contiguous buffer to store the PDH blob. */
-	if ((input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE) ||
-	    !access_ok(input.pdh_cert_address, input.pdh_cert_len)) {
+	if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE) {
 		ret = -EFAULT;
 		goto e_free;
 	}
 
 	/* Allocate a physically contiguous buffer to store the cert chain blob. */
-	if ((input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE) ||
-	    !access_ok(input.cert_chain_address, input.cert_chain_len)) {
+	if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE) {
 		ret = -EFAULT;
 		goto e_free;
 	}
-- 
2.11.0

[PATCH 14/20] via-pmu: don't bother with access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

we are using copy_to_user() for actual copying

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/macintosh/via-pmu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/macintosh/via-pmu.c b/drivers/macintosh/via-pmu.c
index 83eb05bf85ff..8450d7c008d0 100644
--- a/drivers/macintosh/via-pmu.c
+++ b/drivers/macintosh/via-pmu.c
@@ -2184,8 +2184,6 @@ pmu_read(struct file *file, char __user *buf,
 
 	if (count < 1 || !pp)
 		return -EINVAL;
-	if (!access_ok(buf, count))
-		return -EFAULT;
 
 	spin_lock_irqsave(&pp->lock, flags);
 	add_wait_queue(&pp->wait, &wait);
-- 
2.11.0

[PATCH 15/20] drm_read(): get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

address is passed only to copy_to_user()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/gpu/drm/drm_file.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index eb009d3ab48f..6a1f6c802415 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -569,9 +569,6 @@ ssize_t drm_read(struct file *filp, char __user *buffer,
 	struct drm_device *dev = file_priv->minor->dev;
 	ssize_t ret;
 
-	if (!access_ok(buffer, count))
-		return -EFAULT;
-
 	ret = mutex_lock_interruptible(&file_priv->event_read_lock);
 	if (ret)
 		return ret;
-- 
2.11.0

[PATCH 16/20] efi_test: get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

really, people - get_user(), copy_from_user(), memdup_user(), etc.
all fail if access_ok() does.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/firmware/efi/test/efi_test.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c
index 7baf48c01e72..ddf9eae396fe 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -70,9 +70,6 @@ copy_ucs2_from_user_len(efi_char16_t **dst, efi_char16_t __user *src,
 		return 0;
 	}
 
-	if (!access_ok(src, 1))
-		return -EFAULT;
-
 	buf = memdup_user(src, len);
 	if (IS_ERR(buf)) {
 		*dst = NULL;
@@ -91,9 +88,6 @@ copy_ucs2_from_user_len(efi_char16_t **dst, efi_char16_t __user *src,
 static inline int
 get_ucs2_strsize_from_user(efi_char16_t __user *src, size_t *len)
 {
-	if (!access_ok(src, 1))
-		return -EFAULT;
-
 	*len = user_ucs2_strsize(src);
 	if (*len == 0)
 		return -EFAULT;
@@ -118,9 +112,6 @@ copy_ucs2_from_user(efi_char16_t **dst, efi_char16_t __user *src)
 {
 	size_t len;
 
-	if (!access_ok(src, 1))
-		return -EFAULT;
-
 	len = user_ucs2_strsize(src);
 	if (len == 0)
 		return -EFAULT;
@@ -142,9 +133,6 @@ copy_ucs2_to_user_len(efi_char16_t __user *dst, efi_char16_t *src, size_t len)
 	if (!src)
 		return 0;
 
-	if (!access_ok(dst, 1))
-		return -EFAULT;
-
 	return copy_to_user(dst, src, len);
 }
 
-- 
2.11.0

[PATCH 17/20] lpfc_debugfs: get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

	No, you do NOT need to "protect copy from user" that way.
Incidentally, your userland ABI stinks.  I understand that you
wanted to accept "reset" and "reset\n" as equivalent, but I suspect
that accepting "reset this, you !@^!@!" had been an accident.
Nothing to do about that now - it is a userland ABI...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/scsi/lpfc/lpfc_debugfs.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_debugfs.c b/drivers/scsi/lpfc/lpfc_debugfs.c
index 8a6e02aa553f..5a754fb5f854 100644
--- a/drivers/scsi/lpfc/lpfc_debugfs.c
+++ b/drivers/scsi/lpfc/lpfc_debugfs.c
@@ -2166,10 +2166,6 @@ lpfc_debugfs_lockstat_write(struct file *file, const char __user *buf,
 	char *pbuf;
 	int i;
 
-	/* Protect copy from user */
-	if (!access_ok(buf, nbytes))
-		return -EFAULT;
-
 	memset(mybuf, 0, sizeof(mybuf));
 
 	if (copy_from_user(mybuf, buf, nbytes))
@@ -2621,10 +2617,6 @@ lpfc_debugfs_multixripools_write(struct file *file, const char __user *buf,
 	if (nbytes > 64)
 		nbytes = 64;
 
-	/* Protect copy from user */
-	if (!access_ok(buf, nbytes))
-		return -EFAULT;
-
 	memset(mybuf, 0, sizeof(mybuf));
 
 	if (copy_from_user(mybuf, buf, nbytes))
@@ -2787,10 +2779,6 @@ lpfc_debugfs_scsistat_write(struct file *file, const char __user *buf,
 	char mybuf[6] = {0};
 	int i;
 
-	/* Protect copy from user */
-	if (!access_ok(buf, nbytes))
-		return -EFAULT;
-
 	if (copy_from_user(mybuf, buf, (nbytes >= sizeof(mybuf)) ?
 				       (sizeof(mybuf) - 1) : nbytes))
 		return -EFAULT;
-- 
2.11.0

[PATCH 18/20] usb: get rid of pointless access_ok() calls

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

in all affected cases addresses are passed only to
copy_from()_user or copy_to_user().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/usb/core/devices.c          | 2 --
 drivers/usb/core/devio.c            | 9 ---------
 drivers/usb/gadget/function/f_hid.c | 6 ------
 3 files changed, 17 deletions(-)

diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index 44f28a114c2b..94b6fa6e585e 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -598,8 +598,6 @@ static ssize_t usb_device_read(struct file *file, char __user *buf,
 		return -EINVAL;
 	if (nbytes <= 0)
 		return 0;
-	if (!access_ok(buf, nbytes))
-		return -EFAULT;
 
 	mutex_lock(&usb_bus_idr_lock);
 	/* print devices for all busses */
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6833c918abce..544769807ab8 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1127,11 +1127,6 @@ static int proc_control(struct usb_dev_state *ps, void __user *arg)
 		ctrl.bRequestType, ctrl.bRequest, ctrl.wValue,
 		ctrl.wIndex, ctrl.wLength);
 	if (ctrl.bRequestType & 0x80) {
-		if (ctrl.wLength && !access_ok(ctrl.data,
-					       ctrl.wLength)) {
-			ret = -EINVAL;
-			goto done;
-		}
 		pipe = usb_rcvctrlpipe(dev, 0);
 		snoop_urb(dev, NULL, pipe, ctrl.wLength, tmo, SUBMIT, NULL, 0);
 
@@ -1216,10 +1211,6 @@ static int proc_bulk(struct usb_dev_state *ps, void __user *arg)
 	}
 	tmo = bulk.timeout;
 	if (bulk.ep & 0x80) {
-		if (len1 && !access_ok(bulk.data, len1)) {
-			ret = -EINVAL;
-			goto done;
-		}
 		snoop_urb(dev, NULL, pipe, len1, tmo, SUBMIT, NULL, 0);
 
 		usb_unlock_device(dev);
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index f3816a5c861e..df671acdd464 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -252,9 +252,6 @@ static ssize_t f_hidg_read(struct file *file, char __user *buffer,
 	if (!count)
 		return 0;
 
-	if (!access_ok(buffer, count))
-		return -EFAULT;
-
 	spin_lock_irqsave(&hidg->read_spinlock, flags);
 
 #define READ_COND (!list_empty(&hidg->completed_out_req))
@@ -339,9 +336,6 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	unsigned long flags;
 	ssize_t status = -ENOMEM;
 
-	if (!access_ok(buffer, count))
-		return -EFAULT;
-
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 
 #define WRITE_COND (!hidg->write_pending)
-- 
2.11.0

[PATCH 19/20] hfi1: get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

pin_user_pages_fast() doesn't need that from its caller.
NB: only reachable from ->ioctl(), and only under USER_DS

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 4da03f823474..f81ca20f4b69 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -206,13 +206,6 @@ static int pin_rcv_pages(struct hfi1_filedata *fd, struct tid_user_buf *tidbuf)
 		return -EINVAL;
 	}
 
-	/* Verify that access is OK for the user buffer */
-	if (!access_ok((void __user *)vaddr,
-		       npages * PAGE_SIZE)) {
-		dd_dev_err(dd, "Fail vaddr %p, %u pages, !access_ok\n",
-			   (void *)vaddr, npages);
-		return -EFAULT;
-	}
 	/* Allocate the array of struct page pointers needed for pinning */
 	pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
 	if (!pages)
-- 
2.11.0

[PATCH 4/4] vmci_host: get rid of pointless access_ok()

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

get_user_pages_fast() doesn't need the caller to check that.
NB: reachable only from ioctl(2) and only under USER_DS

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/misc/vmw_vmci/vmci_host.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/misc/vmw_vmci/vmci_host.c b/drivers/misc/vmw_vmci/vmci_host.c
index ce16d6b99295..2d8328d928d5 100644
--- a/drivers/misc/vmw_vmci/vmci_host.c
+++ b/drivers/misc/vmw_vmci/vmci_host.c
@@ -233,8 +233,6 @@ static int vmci_host_setup_notify(struct vmci_ctx *context,
 	 * about the size.
 	 */
 	BUILD_BUG_ON(sizeof(bool) != sizeof(u8));
-	if (!access_ok((void __user *)uva, sizeof(u8)))
-		return VMCI_ERROR_GENERIC;
 
 	/*
 	 * Lock physical page backing a given user VA.
-- 
2.11.0

Re: [PATCHES] uaccess simple access_ok() removals

[Linus Torvalds]

On Sat, May 9, 2020 at 4:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         Individual patches in followups; if nobody screams - into #for-next
> it goes...

Looks fine to me, although I only read your commit logs, I didn't
verify that what you stated was actually true (ie the whole "only used
for xyz" parts).

But I'll take your word for it. Particularly the double-underscore
versions are getting rare (and presumably some of the other branches
you have make it rarer still).

               Linus

Re: [PATCH 05/20] tomoyo_write_control(): get rid of pointless access_ok()

[Tetsuo Handa]

Hello, Al.

I think that this access_ok() check helps reducing partial writes (either
"whole amount was processed" or "not processed at all" unless -ENOMEM).
Do you think that such attempt is pointless? Then, please go ahead...

On 2020/05/10 8:45, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> address is passed only to get_user()
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  security/tomoyo/common.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index 1b467381986f..f93f8acd05f7 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -2662,8 +2662,6 @@ ssize_t tomoyo_write_control(struct tomoyo_io_buffer *head,
>  
>  	if (!head->write)
>  		return -EINVAL;
> -	if (!access_ok(buffer, buffer_len))
> -		return -EFAULT;
>  	if (mutex_lock_interruptible(&head->io_sem))
>  		return -EINTR;
>  	head->read_user_buf_avail = 0;
> 

Re: [PATCH 05/20] tomoyo_write_control(): get rid of pointless access_ok()

[Linus Torvalds]

On Sat, May 9, 2020 at 5:51 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> I think that this access_ok() check helps reducing partial writes (either
> "whole amount was processed" or "not processed at all" unless -ENOMEM).

No it doesn't.

"access_ok()" only checks the range being a valid user address range.

It doesn't actually help at all if the worry is "what if we take a
page fault in the middle".  Because it simply doesn't check those
kinds of things.

Now, if somebody passes actual invalid ranges (ie kernel addresses or
other crazy stuff), they only have themselves to blame. The invalid
range will be noticed when actually doing the user copy, and then
you'll get EFAULT there. But there's no point in trying to figure that
out early - it's only adding overhead, and it doesn't help any normal
case.

                  Linus

Re: [PATCH 05/20] tomoyo_write_control(): get rid of pointless access_ok()

[Tetsuo Handa]

On 2020/05/10 9:57, Linus Torvalds wrote:
> On Sat, May 9, 2020 at 5:51 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>
>> I think that this access_ok() check helps reducing partial writes (either
>> "whole amount was processed" or "not processed at all" unless -ENOMEM).
> 
> No it doesn't.
> 
> "access_ok()" only checks the range being a valid user address range.
> 

I see. Thank you.

Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Re: [PATCH 05/20] tomoyo_write_control(): get rid of pointless access_ok()

[Al Viro]

On Sat, May 09, 2020 at 05:57:56PM -0700, Linus Torvalds wrote:
> On Sat, May 9, 2020 at 5:51 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >
> > I think that this access_ok() check helps reducing partial writes (either
> > "whole amount was processed" or "not processed at all" unless -ENOMEM).
> 
> No it doesn't.
> 
> "access_ok()" only checks the range being a valid user address range.
> 
> It doesn't actually help at all if the worry is "what if we take a
> page fault in the middle".  Because it simply doesn't check those
> kinds of things.
> 
> Now, if somebody passes actual invalid ranges (ie kernel addresses or
> other crazy stuff), they only have themselves to blame. The invalid
> range will be noticed when actually doing the user copy, and then
> you'll get EFAULT there. But there's no point in trying to figure that
> out early - it's only adding overhead, and it doesn't help any normal
> case.

It might be a good idea to add Documentation/what-access_ok-does_not ;-/
In addition to what you've mentioned,

* access_ok() does not fault anything in; never had.

* access_ok() does not verify that memory is readable/writable/there at all;
never had, except for genuine 80386 and (maybe) some of the shittier 486
clones.

* access_ok() does not protect you from the length being insanely large;
even on i386 it can pass with length being a bit under 3Gb.  If you
count upon it to prevent kmalloc() complaints about insanely large
allocation (yes, I've seen that excuse used), you are wrong.

* on a bunch of architectures access_ok() never rejects anything, and
no, that's _not_ MMU-less ones.  sparc64, for example.  Or s390, or
parisc, etc.

Re: [PATCHES] uaccess simple access_ok() removals

[Al Viro]

On Sat, May 09, 2020 at 05:34:58PM -0700, Linus Torvalds wrote:
> On Sat, May 9, 2020 at 4:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> >         Individual patches in followups; if nobody screams - into #for-next
> > it goes...
> 
> Looks fine to me, although I only read your commit logs, I didn't
> verify that what you stated was actually true (ie the whole "only used
> for xyz" parts).
> 
> But I'll take your word for it. Particularly the double-underscore
> versions are getting rare (and presumably some of the other branches
> you have make it rarer still).

I have - FWIW, right now I'm going through the patch series for netdev;
once I'm done with turning commit messages into something printable
(and finish rereading the patches themselves), there it goes, hopefully
tonight.  Then a bunch of small branches + repost of csum one + some
of the weird shit (comedi compat ioctls - the largest pile of
__get_user() and __put_user() outside of arch/* is festering there).
Then regset stuff + (probably) resurrection of i915 stuff.

I've got a bunch of stuff around execve(), but that'll have to wait
until I get through the recently posted execve-related patchsets...

Then there's e.g. mempolicy compat stuff, etc., but I'd rather wait
for several days before posting that, reviewers' bandwidth being
finite...

Re: [PATCH 04/20] FIEMAP: don't bother with access_ok()

[Christoph Hellwig]

On Sun, May 10, 2020 at 12:45:41AM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> we use copy_to_user() on that thing anyway (and always had).

I already have this patch in this series:

https://lore.kernel.org/linux-fsdevel/20200507145924.GA28854@lst.de/T/#t

which is waiting to be picked up [1], and also has some chance for conflicts
due to changes next to the access_ok.

[1] except for the first two patches, which Ted plans to send for 5.7

RE: [PATCHES] uaccess simple access_ok() removals

[David Laight]

From: Al Viro
> Sent: 10 May 2020 00:41
> 
> 	One of the uaccess-related branches; this one is just the
> cases when access_ok() calls are trivially pointless - the address
> in question gets fed only to primitives that do access_ok() checks
> themselves.

There is also the check in rw_copy_check_uvector() that should
always be replicated by the copy_to/from_user() in _copy_to/from_iter().

And the strange call to rw_copy_check_uvector() in mm/process_vm_access.c
which carefully avoids the access_ok() check for the target process.
I did a quick look, but failed to see an obvious check further
down the call path.
The code is doing a read/write from another process, not sure when it
is used - not by gdb.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 04/20] FIEMAP: don't bother with access_ok()

[Al Viro]

On Sun, May 10, 2020 at 12:02:41AM -0700, Christoph Hellwig wrote:
> On Sun, May 10, 2020 at 12:45:41AM +0100, Al Viro wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > we use copy_to_user() on that thing anyway (and always had).
> 
> I already have this patch in this series:
> 
> https://lore.kernel.org/linux-fsdevel/20200507145924.GA28854@lst.de/T/#t
> 
> which is waiting to be picked up [1], and also has some chance for conflicts
> due to changes next to the access_ok.
> 
> [1] except for the first two patches, which Ted plans to send for 5.7

I can drop this commit, of course, it's not a prereq for anything else in there.
Or I could pick your series into never-rebased branch, but it would complicate
the life wrt ext4 tree - up to you and Ted...

Re: [PATCH 04/20] FIEMAP: don't bother with access_ok()

[Christoph Hellwig]

On Wed, May 13, 2020 at 08:02:07PM +0100, Al Viro wrote:
> > https://lore.kernel.org/linux-fsdevel/20200507145924.GA28854@lst.de/T/#t
> > 
> > which is waiting to be picked up [1], and also has some chance for conflicts
> > due to changes next to the access_ok.
> > 
> > [1] except for the first two patches, which Ted plans to send for 5.7
> 
> I can drop this commit, of course, it's not a prereq for anything else in there.
> Or I could pick your series into never-rebased branch, but it would complicate
> the life wrt ext4 tree - up to you and Ted...

I really don't care - the first two really need to go in ASAP and
Ted promised to pick them up, but I've not seen them in linux-next
yet.  The rest can go wherever once the first ones hit mainline.

Re: [PATCH 12/20] omapfb: get rid of pointless access_ok() calls

[Bartlomiej Zolnierkiewicz]

On 5/10/20 1:45 AM, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> address is passed only to copy_to_user()
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> ---
>  drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c b/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c
> index 56995f44e76d..f40be68d5aac 100644
> --- a/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c
> +++ b/drivers/video/fbdev/omap2/omapfb/omapfb-ioctl.c
> @@ -482,9 +482,6 @@ static int omapfb_memory_read(struct fb_info *fbi,
>  	if (!display || !display->driver->memory_read)
>  		return -ENOENT;
>  
> -	if (!access_ok(mr->buffer, mr->buffer_size))
> -		return -EFAULT;
> -
>  	if (mr->w > 4096 || mr->h > 4096)
>  		return -EINVAL;
>  
> 

Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls

[Bartlomiej Zolnierkiewicz]

Hi Al,

On 5/10/20 1:45 AM, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> addresses passed only to get_user() and put_user()

This driver lacks checks for {get,put}_user() return values so it will
now return 0 ("success") even if {get,put}_user() fails.

Am I missing something?

> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/video/fbdev/amifb.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c
> index 20e03e00b66d..6062104f3afb 100644
> --- a/drivers/video/fbdev/amifb.c
> +++ b/drivers/video/fbdev/amifb.c
> @@ -1855,8 +1855,6 @@ static int ami_get_var_cursorinfo(struct fb_var_cursorinfo *var,
>  	var->yspot = par->crsr.spot_y;
>  	if (size > var->height * var->width)
>  		return -ENAMETOOLONG;
> -	if (!access_ok(data, size))
> -		return -EFAULT;
>  	delta = 1 << par->crsr.fmode;
>  	lspr = lofsprite + (delta << 1);
>  	if (par->bplcon0 & BPC0_LACE)
> @@ -1935,8 +1933,6 @@ static int ami_set_var_cursorinfo(struct fb_var_cursorinfo *var,
>  		return -EINVAL;
>  	if (!var->height)
>  		return -EINVAL;
> -	if (!access_ok(data, var->width * var->height))
> -		return -EFAULT;
>  	delta = 1 << fmode;
>  	lofsprite = shfsprite = (u_short *)spritememory;
>  	lspr = lofsprite + (delta << 1);
> 
 
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls

[Al Viro]

On Thu, May 14, 2020 at 03:45:09PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi Al,
> 
> On 5/10/20 1:45 AM, Al Viro wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > addresses passed only to get_user() and put_user()
> 
> This driver lacks checks for {get,put}_user() return values so it will
> now return 0 ("success") even if {get,put}_user() fails.
> 
> Am I missing something?

"now" is interesting, considering
/* We let the MMU do all checking */
static inline int access_ok(const void __user *addr,
                            unsigned long size)
{
        return 1;
}
in arch/m68k/include/asm/uaccess_mm.h

Again, access_ok() is *NOT* about checking if memory is readable/writable/there
in the first place.  All it does is a static check that address is in
"userland" range - on architectures that have kernel and userland sharing the
address space.  On architectures where we have separate ASI or equivalents
thereof for kernel and for userland the fscker is always true.

If MMU will prevent access to kernel memory by uaccess insns for given address
range, access_ok() is fine with it.  It does not do anything else.

And yes, get_user()/put_user() callers should handle the fact that those can
fail.  Which they bloody well can _after_ _success_ of access_ok().  And
without any races whatsoever.

IOW, the lack of such checks is a bug, but it's quite independent from the
bogus access_ok() call.  On any architecture.  mmap() something, munmap()
it and pass the address where it used to be to that ioctl().  Failing
get_user()/put_user() is guaranteed, so's succeeding access_ok().

And that code is built only on amiga, so access_ok() always succeeds, anyway.

Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls

[Bartlomiej Zolnierkiewicz]

On 5/14/20 4:07 PM, Al Viro wrote:
> On Thu, May 14, 2020 at 03:45:09PM +0200, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Al,
>>
>> On 5/10/20 1:45 AM, Al Viro wrote:
>>> From: Al Viro <viro@zeniv.linux.org.uk>
>>>
>>> addresses passed only to get_user() and put_user()
>>
>> This driver lacks checks for {get,put}_user() return values so it will
>> now return 0 ("success") even if {get,put}_user() fails.
>>
>> Am I missing something?
> 
> "now" is interesting, considering
> /* We let the MMU do all checking */
> static inline int access_ok(const void __user *addr,
>                             unsigned long size)
> {
>         return 1;
> }
> in arch/m68k/include/asm/uaccess_mm.h
> 
> Again, access_ok() is *NOT* about checking if memory is readable/writable/there
> in the first place.  All it does is a static check that address is in
> "userland" range - on architectures that have kernel and userland sharing the
> address space.  On architectures where we have separate ASI or equivalents
> thereof for kernel and for userland the fscker is always true.
> 
> If MMU will prevent access to kernel memory by uaccess insns for given address
> range, access_ok() is fine with it.  It does not do anything else.
> 
> And yes, get_user()/put_user() callers should handle the fact that those can
> fail.  Which they bloody well can _after_ _success_ of access_ok().  And
> without any races whatsoever.
> 
> IOW, the lack of such checks is a bug, but it's quite independent from the
> bogus access_ok() call.  On any architecture.  mmap() something, munmap()
> it and pass the address where it used to be to that ioctl().  Failing
> get_user()/put_user() is guaranteed, so's succeeding access_ok().
> 
> And that code is built only on amiga, so access_ok() always succeeds, anyway.

Thank you for in-detail explanations, for this patch:

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Could you also please take care of adding missing checks for {get,put}_user()
failures later?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls

[Al Viro]

On Thu, May 14, 2020 at 04:25:35PM +0200, Bartlomiej Zolnierkiewicz wrote:
> Thank you for in-detail explanations, for this patch:
> 
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> 
> Could you also please take care of adding missing checks for {get,put}_user()
> failures later?

Umm...  OK; put_user() side is trivial -  the interesting part is what to do
about get_user() failures halfway through.  Right now it treats them as
"we'd read zeroes".  On anything else I would say "screw it, memdup_user()
the damn thing on the way in and copy from there", but... Amiga has how
much RAM, again?

OTOH, from my reading of that code it does appear to be limited to
4Kb of data to copy, so it's probably OK...  Hell knows - I'm really
confused by those #ifdef __mc68000__ in there; the driver *is*
amiga-only:
obj-$(CONFIG_FB_AMIGA)            += amifb.o c2p_planar.o
config FB_AMIGA
        tristate "Amiga native chipset support"
        depends on FB && AMIGA
and AMIGA is defined only in arch/m68k/Kconfig.machine.  So how the
hell can it *not* be true?  OTOH, it looks like hand-optimized
asm equivalents of C they have in #else, so that #else might be
meant to document what's going on...

I've no idea how to test any changes to that thing - the only
m68k emulator I'm reasonably familiar with is aranym, and
that's Atari, not Amiga.  Never got around to setting up UAE...
So I can do a patch more or less blindly (memdup_user() after
it has checked the limits on height/width, then dereferencing
from copy instead of get_user()), but I won't be able to test
it.

Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls

[Geert Uytterhoeven]

Hi Al,

On Thu, May 14, 2020 at 7:43 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, May 14, 2020 at 04:25:35PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > Thank you for in-detail explanations, for this patch:
> >
> > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> >
> > Could you also please take care of adding missing checks for {get,put}_user()
> > failures later?
>
> Umm...  OK; put_user() side is trivial -  the interesting part is what to do
> about get_user() failures halfway through.  Right now it treats them as
> "we'd read zeroes".  On anything else I would say "screw it, memdup_user()
> the damn thing on the way in and copy from there", but... Amiga has how
> much RAM, again?

In theory, up to 3.5 GiB ;-)
In practice, 16 MiB is already a lot (mine has 12).

> OTOH, from my reading of that code it does appear to be limited to
> 4Kb of data to copy, so it's probably OK...  Hell knows - I'm really
> confused by those #ifdef __mc68000__ in there; the driver *is*
> amiga-only:
> obj-$(CONFIG_FB_AMIGA)            += amifb.o c2p_planar.o
> config FB_AMIGA
>         tristate "Amiga native chipset support"
>         depends on FB && AMIGA
> and AMIGA is defined only in arch/m68k/Kconfig.machine.  So how the
> hell can it *not* be true?  OTOH, it looks like hand-optimized
> asm equivalents of C they have in #else, so that #else might be
> meant to document what's going on...

These #ifdefs are relics from APUS (Amiga Power-Up System), which
added a PPC board.  APUS support was killed off a long time ago,
when arch/ppc/ was still king, but these #ifdefs were missed, because
they didn't test for CONFIG_APUS.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 06/20] n_hdlc_tty_read(): remove pointless access_ok()

[Greg Kroah-Hartman]

On Sun, May 10, 2020 at 12:45:43AM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> only copy_to_user() is done to the address in question
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/tty/n_hdlc.c | 7 -------
>  1 file changed, 7 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 18/20] usb: get rid of pointless access_ok() calls

[Greg Kroah-Hartman]

On Sun, May 10, 2020 at 12:45:55AM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> in all affected cases addresses are passed only to
> copy_from()_user or copy_to_user().
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/usb/core/devices.c          | 2 --
>  drivers/usb/core/devio.c            | 9 ---------
>  drivers/usb/gadget/function/f_hid.c | 6 ------
>  3 files changed, 17 deletions(-)


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 4/4] vmci_host: get rid of pointless access_ok()

[Greg Kroah-Hartman]

On Sun, May 10, 2020 at 12:45:57AM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> get_user_pages_fast() doesn't need the caller to check that.
> NB: reachable only from ioctl(2) and only under USER_DS
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/misc/vmw_vmci/vmci_host.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 07/20] nvram: drop useless access_ok()

[Greg Kroah-Hartman]

On Sun, May 10, 2020 at 12:45:44AM +0100, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> we are using copy_to_user()/memdup_user() anyway
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/char/nvram.c | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 04/20] FIEMAP: don't bother with access_ok()

[Al Viro]

On Wed, May 13, 2020 at 12:38:01PM -0700, Christoph Hellwig wrote:
> On Wed, May 13, 2020 at 08:02:07PM +0100, Al Viro wrote:
> > > https://lore.kernel.org/linux-fsdevel/20200507145924.GA28854@lst.de/T/#t
> > > 
> > > which is waiting to be picked up [1], and also has some chance for conflicts
> > > due to changes next to the access_ok.
> > > 
> > > [1] except for the first two patches, which Ted plans to send for 5.7
> > 
> > I can drop this commit, of course, it's not a prereq for anything else in there.
> > Or I could pick your series into never-rebased branch, but it would complicate
> > the life wrt ext4 tree - up to you and Ted...
> 
> I really don't care - the first two really need to go in ASAP and
> Ted promised to pick them up, but I've not seen them in linux-next
> yet.  The rest can go wherever once the first ones hit mainline.

OK, dropped