linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Linux Media Mailing List <linux-media@vger.kernel.org>
Cc: Dillon Min <dillon.minfei@gmail.com>
Subject: [RFC PATCH] media: check for NULL pointer argument in ioctl() if !CONFIG_MMU
Date: Thu, 14 Oct 2021 13:20:28 +0200	[thread overview]
Message-ID: <3acd9ee4-5a58-6ed4-17fe-61596a5252b8@xs4all.nl> (raw)

If CONFIG_MMU is not set, then copying ioctl arguments from userspace to kernelspace
and vice versa will just work (access_ok() always returns true in that case), even if
the argument is a NULL pointer.

This is definitely a corner case that we want to check for, so add a NULL pointer check
to the various core ioctl functions in the media frameworks.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: Dillon Min <dillon.minfei@gmail.com>
---
Note: this is an RFC only, this might fail if there are ioctls that pass a value as
the ioctl argument instead of a pointer to a buffer. I know that that never happens for
the V4L2, CEC and MC APIs, but I'm less certain about the DVB/RC APIs.

Dillon, can you test if the v4l2-compiance VIDIOC_QUERYCAP(NULL) test now passes with
this patch applied?

Thanks!

	Hans
---
diff --git a/drivers/media/cec/core/cec-api.c b/drivers/media/cec/core/cec-api.c
index 769e6b4cddce..b2498f0dd272 100644
--- a/drivers/media/cec/core/cec-api.c
+++ b/drivers/media/cec/core/cec-api.c
@@ -511,6 +511,11 @@ static long cec_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (!cec_is_registered(adap))
 		return -ENODEV;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
+
 	switch (cmd) {
 	case CEC_ADAP_G_CAPS:
 		return cec_adap_g_caps(adap, parg);
diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
index 5d5a48475a54..1ab0fe8df2d6 100644
--- a/drivers/media/dvb-core/dmxdev.c
+++ b/drivers/media/dvb-core/dmxdev.c
@@ -1187,6 +1187,10 @@ static int dvb_demux_do_ioctl(struct file *file,
 static long dvb_demux_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {
+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	return dvb_usercopy(file, cmd, arg, dvb_demux_do_ioctl);
 }

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c
index 15a08d8c69ef..8ed33806fe13 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -1422,6 +1422,10 @@ static int dvb_ca_en50221_io_do_ioctl(struct file *file,
 static long dvb_ca_en50221_io_ioctl(struct file *file,
 				    unsigned int cmd, unsigned long arg)
 {
+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	return dvb_usercopy(file, cmd, arg, dvb_ca_en50221_io_do_ioctl);
 }

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 258637d762d6..b21f28d6d6df 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2100,6 +2100,10 @@ static long dvb_frontend_ioctl(struct file *file, unsigned int cmd,
 	if (!dvbdev)
 		return -ENODEV;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	return dvb_usercopy(file, cmd, arg, dvb_frontend_do_ioctl);
 }

@@ -2136,6 +2140,10 @@ static int dvb_frontend_handle_compat_ioctl(struct file *file, unsigned int cmd,
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 	int i, err = 0;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	if (cmd == COMPAT_FE_SET_PROPERTY) {
 		struct compat_dtv_properties prop, *tvps = NULL;
 		struct compat_dtv_property *tvp = NULL;
diff --git a/drivers/media/dvb-core/dvb_net.c b/drivers/media/dvb-core/dvb_net.c
index dddebea644bb..97282946e35e 100644
--- a/drivers/media/dvb-core/dvb_net.c
+++ b/drivers/media/dvb-core/dvb_net.c
@@ -1561,6 +1561,10 @@ static int dvb_net_do_ioctl(struct file *file,
 static long dvb_net_ioctl(struct file *file,
 	      unsigned int cmd, unsigned long arg)
 {
+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	return dvb_usercopy(file, cmd, arg, dvb_net_do_ioctl);
 }

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index 795d9bfaba5c..f2311baadb00 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -184,6 +184,10 @@ long dvb_generic_ioctl(struct file *file,
 	if (!dvbdev)
 		return -ENODEV;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	if (!dvbdev->kernel_ioctl)
 		return -EINVAL;

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index cf5e459b1d96..e25a407b6194 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -445,6 +445,10 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 	char __karg[256], *karg = __karg;
 	long ret;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !__arg)
+		return -EFAULT;
+#endif
 	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
 	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
 		return -ENOIOCTLCMD;
@@ -526,6 +530,10 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 	struct media_device *dev = devnode->media_dev;
 	long ret;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	switch (cmd) {
 	case MEDIA_IOC_ENUM_LINKS32:
 		mutex_lock(&dev->graph_mutex);
diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
index addb8f2d8939..0c1956899fcb 100644
--- a/drivers/media/mc/mc-request.c
+++ b/drivers/media/mc/mc-request.c
@@ -223,6 +223,10 @@ static long media_request_ioctl(struct file *filp, unsigned int cmd,
 {
 	struct media_request *req = filp->private_data;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	switch (cmd) {
 	case MEDIA_REQUEST_IOC_QUEUE:
 		return media_request_ioctl_queue(req);
diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 85fcdc59f0d1..f7bd7a07606e 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -532,6 +532,10 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 	void __user *arg = (void __user *)ioctl_arg;
 	int result = 0;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !ioctl_arg)
+		return -EFAULT;
+#endif
 	mutex_lock(&dst_ca_mutex);
 	dvbdev = file->private_data;
 	state = (struct dst_state *)dvbdev->priv;
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 7f591ff5269d..9d89c68d2076 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -372,6 +372,10 @@ static long lirc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	u32 val = 0;
 	int ret;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
 	if (_IOC_DIR(cmd) & _IOC_WRITE) {
 		ret = get_user(val, argp);
 		if (ret)
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 8176769a89fa..2bb8fa87aa46 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -1254,6 +1254,11 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 	if (!video_is_registered(vdev))
 		return -ENODEV;

+#ifndef CONFIG_MMU
+	if (_IOC_DIR(cmd) != _IOC_NONE && !arg)
+		return -EFAULT;
+#endif
+
 	if (_IOC_TYPE(cmd) == 'V' && _IOC_NR(cmd) < BASE_VIDIOC_PRIVATE)
 		ret = file->f_op->unlocked_ioctl(file, cmd,
 					(unsigned long)compat_ptr(arg));
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d0109ce5a8..d4ec18fd1770 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3279,6 +3279,10 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,

 	/*  Copy arguments into temp kernel buffer  */
 	if (_IOC_DIR(cmd) != _IOC_NONE) {
+#ifndef CONFIG_MMU
+		if (!arg)
+			return -EFAULT;
+#endif
 		if (ioc_size <= sizeof(sbuf)) {
 			parg = sbuf;
 		} else {

             reply	other threads:[~2021-10-14 11:20 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-14 11:20 Hans Verkuil [this message]
2021-10-14 14:02 ` Dillon Min
2021-10-15 10:25   ` Dillon Min

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3acd9ee4-5a58-6ed4-17fe-61596a5252b8@xs4all.nl \
    --to=hverkuil-cisco@xs4all.nl \
    --cc=dillon.minfei@gmail.com \
    --cc=linux-media@vger.kernel.org \
    --subject='Re: [RFC PATCH] media: check for NULL pointer argument in ioctl() if '\!'CONFIG_MMU' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).