* [RFC PATCH] media: check for NULL pointer argument in ioctl() if !CONFIG_MMU
@ 2021-10-14 11:20 Hans Verkuil
2021-10-14 14:02 ` Dillon Min
0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2021-10-14 11:20 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Dillon Min
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 {
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] media: check for NULL pointer argument in ioctl() if !CONFIG_MMU
2021-10-14 11:20 [RFC PATCH] media: check for NULL pointer argument in ioctl() if !CONFIG_MMU Hans Verkuil
@ 2021-10-14 14:02 ` Dillon Min
2021-10-15 10:25 ` Dillon Min
0 siblings, 1 reply; 3+ messages in thread
From: Dillon Min @ 2021-10-14 14:02 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List
On Thu, 14 Oct 2021 at 19:20, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> 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-compliance VIDIOC_QUERYCAP(NULL) test now passes with
> this patch applied?
Sure, I will test this patch.
Best Regards
Dillon
>
> 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 {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] media: check for NULL pointer argument in ioctl() if !CONFIG_MMU
2021-10-14 14:02 ` Dillon Min
@ 2021-10-15 10:25 ` Dillon Min
0 siblings, 0 replies; 3+ messages in thread
From: Dillon Min @ 2021-10-15 10:25 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List
Hi Hans
Your patch working fine on my setup:
HW: stm32f469-disco https://www.st.com/en/evaluation-tools/32f469idiscovery.html
The detail test step:
- rebase kernel on the media tree from
https://git.linuxtv.org/media_tree.git master branch.
commit 64cdf7e5a3aac0e7c9efdb079e74e22875b0419a
Author: Irui Wang <irui.wang@mediatek.com>
Date: Sun Sep 26 05:39:35 2021 +0200
- merge my 8 patches.
- open nullptr check from v4l2-compliance.
- build kernel & v4l2-compliance.
- v4l2-compliance test failed as before.
- merge your patch, rebuild kernel
- the "fail: v4l2-compliance.cpp(624): doioctl(node, VIDIOC_QUERYCAP,
nullptr) != EFAULT"
is gone.
Thanks & Regards
Dillon
On Thu, 14 Oct 2021 at 22:02, Dillon Min <dillon.minfei@gmail.com> wrote:
>
> On Thu, 14 Oct 2021 at 19:20, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >
> > 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-compliance VIDIOC_QUERYCAP(NULL) test now passes with
> > this patch applied?
>
> Sure, I will test this patch.
>
> Best Regards
> Dillon
>
> >
> > 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 {
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-15 10:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14 11:20 [RFC PATCH] media: check for NULL pointer argument in ioctl() if !CONFIG_MMU Hans Verkuil
2021-10-14 14:02 ` Dillon Min
2021-10-15 10:25 ` Dillon Min
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.