* [PATCH v2 0/2] videobuf2: Fix kernel memory overwriting @ 2016-05-12 12:14 Sakari Ailus 2016-05-12 12:14 ` [PATCH v2 1/2] vb2: core: Skip planes array verification if pb is NULL Sakari Ailus 2016-05-12 12:14 ` [PATCH v2 2/2] [media] videobuf2-v4l2: Verify planes array in buffer dequeueing Sakari Ailus 0 siblings, 2 replies; 5+ messages in thread From: Sakari Ailus @ 2016-05-12 12:14 UTC (permalink / raw) To: linux-media; +Cc: mchehab, hverkuil, david, linux-kernel Hi folks, There was a bug in the first version of the set --- the pb argument to __vb2_get_done_vb() is NULL in cases which did not get tested the last time. The second patch in the set was reverted for that reason. This set contains a patch originally from David and again the second reverted patch to fix the memory corruption issue. The previous set can be found here: <URL:http://www.spinics.net/lists/linux-media/msg99336.html> The patches have been tested with V4L2 streaming and file I/O API but not with DVB. -- Kind regards, Sakari ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] vb2: core: Skip planes array verification if pb is NULL 2016-05-12 12:14 [PATCH v2 0/2] videobuf2: Fix kernel memory overwriting Sakari Ailus @ 2016-05-12 12:14 ` Sakari Ailus 2016-05-13 9:09 ` Hans Verkuil 2016-05-12 12:14 ` [PATCH v2 2/2] [media] videobuf2-v4l2: Verify planes array in buffer dequeueing Sakari Ailus 1 sibling, 1 reply; 5+ messages in thread From: Sakari Ailus @ 2016-05-12 12:14 UTC (permalink / raw) To: linux-media; +Cc: mchehab, hverkuil, david, linux-kernel, Sakari Ailus An earlier patch fixing an input validation issue introduced another issue: vb2_core_dqbuf() is called with pb argument value NULL in some cases, causing a NULL pointer dereference. Fix this by skipping the verification as there's nothing to verify. Signed-off-by: David R <david@unsolicited.net> Use if () instead of ? :; it's nicer that way. Improve the comment in the code as well. Fixes: e7e0c3e26587 ("[media] videobuf2-core: Check user space planes array in dqbuf") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: stable@vger.kernel.org # for v4.4 and later --- drivers/media/v4l2-core/videobuf2-core.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 9fbcb67..633fc1a 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -1648,7 +1648,7 @@ static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb, void *pb, int nonblocking) { unsigned long flags; - int ret; + int ret = 0; /* * Wait for at least one buffer to become available on the done_list. @@ -1664,10 +1664,12 @@ static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb, spin_lock_irqsave(&q->done_lock, flags); *vb = list_first_entry(&q->done_list, struct vb2_buffer, done_entry); /* - * Only remove the buffer from done_list if v4l2_buffer can handle all - * the planes. + * Only remove the buffer from done_list if all planes can be + * handled. Some cases such as V4L2 file I/O and DVB have pb + * == NULL; skip the check then as there's nothing to verify. */ - ret = call_bufop(q, verify_planes_array, *vb, pb); + if (pb) + ret = call_bufop(q, verify_planes_array, *vb, pb); if (!ret) list_del(&(*vb)->done_entry); spin_unlock_irqrestore(&q->done_lock, flags); -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] vb2: core: Skip planes array verification if pb is NULL 2016-05-12 12:14 ` [PATCH v2 1/2] vb2: core: Skip planes array verification if pb is NULL Sakari Ailus @ 2016-05-13 9:09 ` Hans Verkuil 2016-05-13 9:38 ` Sakari Ailus 0 siblings, 1 reply; 5+ messages in thread From: Hans Verkuil @ 2016-05-13 9:09 UTC (permalink / raw) To: Sakari Ailus, linux-media; +Cc: mchehab, david, linux-kernel On 05/12/2016 02:14 PM, Sakari Ailus wrote: > An earlier patch fixing an input validation issue introduced another > issue: vb2_core_dqbuf() is called with pb argument value NULL in some > cases, causing a NULL pointer dereference. Fix this by skipping the > verification as there's nothing to verify. > > Signed-off-by: David R <david@unsolicited.net> > > Use if () instead of ? :; it's nicer that way. Improve the comment in the > code as well. This comment doesn't seem applicable to this patch. Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> Hans > > Fixes: e7e0c3e26587 ("[media] videobuf2-core: Check user space planes array in dqbuf") > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: stable@vger.kernel.org # for v4.4 and later > --- > drivers/media/v4l2-core/videobuf2-core.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 9fbcb67..633fc1a 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -1648,7 +1648,7 @@ static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb, > void *pb, int nonblocking) > { > unsigned long flags; > - int ret; > + int ret = 0; > > /* > * Wait for at least one buffer to become available on the done_list. > @@ -1664,10 +1664,12 @@ static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb, > spin_lock_irqsave(&q->done_lock, flags); > *vb = list_first_entry(&q->done_list, struct vb2_buffer, done_entry); > /* > - * Only remove the buffer from done_list if v4l2_buffer can handle all > - * the planes. > + * Only remove the buffer from done_list if all planes can be > + * handled. Some cases such as V4L2 file I/O and DVB have pb > + * == NULL; skip the check then as there's nothing to verify. > */ > - ret = call_bufop(q, verify_planes_array, *vb, pb); > + if (pb) > + ret = call_bufop(q, verify_planes_array, *vb, pb); > if (!ret) > list_del(&(*vb)->done_entry); > spin_unlock_irqrestore(&q->done_lock, flags); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] vb2: core: Skip planes array verification if pb is NULL 2016-05-13 9:09 ` Hans Verkuil @ 2016-05-13 9:38 ` Sakari Ailus 0 siblings, 0 replies; 5+ messages in thread From: Sakari Ailus @ 2016-05-13 9:38 UTC (permalink / raw) To: Hans Verkuil, linux-media; +Cc: mchehab, david, linux-kernel Hi Hans, Hans Verkuil wrote: > On 05/12/2016 02:14 PM, Sakari Ailus wrote: >> An earlier patch fixing an input validation issue introduced another >> issue: vb2_core_dqbuf() is called with pb argument value NULL in some >> cases, causing a NULL pointer dereference. Fix this by skipping the >> verification as there's nothing to verify. >> >> Signed-off-by: David R <david@unsolicited.net> >> >> Use if () instead of ? :; it's nicer that way. Improve the comment in the >> code as well. > > This comment doesn't seem applicable to this patch. Compared to the original patch. I can sure drop the comment as well, it's not that important. > > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> Thanks! -- Sakari Ailus sakari.ailus@linux.intel.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] [media] videobuf2-v4l2: Verify planes array in buffer dequeueing 2016-05-12 12:14 [PATCH v2 0/2] videobuf2: Fix kernel memory overwriting Sakari Ailus 2016-05-12 12:14 ` [PATCH v2 1/2] vb2: core: Skip planes array verification if pb is NULL Sakari Ailus @ 2016-05-12 12:14 ` Sakari Ailus 1 sibling, 0 replies; 5+ messages in thread From: Sakari Ailus @ 2016-05-12 12:14 UTC (permalink / raw) To: linux-media; +Cc: mchehab, hverkuil, david, linux-kernel, Sakari Ailus When a buffer is being dequeued using VIDIOC_DQBUF IOCTL, the exact buffer which will be dequeued is not known until the buffer has been removed from the queue. The number of planes is specific to a buffer, not to the queue. This does lead to the situation where multi-plane buffers may be requested and queued with n planes, but VIDIOC_DQBUF IOCTL may be passed an argument struct with fewer planes. __fill_v4l2_buffer() however uses the number of planes from the dequeued videobuf2 buffer, overwriting kernel memory (the m.planes array allocated in video_usercopy() in v4l2-ioctl.c) if the user provided fewer planes than the dequeued buffer had. Oops! Fixes: b0e0e1f83de3 ("[media] media: videobuf2: Prepare to divide videobuf2") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Cc: stable@vger.kernel.org # for v4.4 and later Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com> --- drivers/media/v4l2-core/videobuf2-v4l2.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c index 0b1b8c7..7f366f1 100644 --- a/drivers/media/v4l2-core/videobuf2-v4l2.c +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c @@ -74,6 +74,11 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer return 0; } +static int __verify_planes_array_core(struct vb2_buffer *vb, const void *pb) +{ + return __verify_planes_array(vb, pb); +} + /** * __verify_length() - Verify that the bytesused value for each plane fits in * the plane length and that the data offset doesn't exceed the bytesused value. @@ -437,6 +442,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, } static const struct vb2_buf_ops v4l2_buf_ops = { + .verify_planes_array = __verify_planes_array_core, .fill_user_buffer = __fill_v4l2_buffer, .fill_vb2_buffer = __fill_vb2_buffer, .copy_timestamp = __copy_timestamp, -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-05-13 9:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-12 12:14 [PATCH v2 0/2] videobuf2: Fix kernel memory overwriting Sakari Ailus 2016-05-12 12:14 ` [PATCH v2 1/2] vb2: core: Skip planes array verification if pb is NULL Sakari Ailus 2016-05-13 9:09 ` Hans Verkuil 2016-05-13 9:38 ` Sakari Ailus 2016-05-12 12:14 ` [PATCH v2 2/2] [media] videobuf2-v4l2: Verify planes array in buffer dequeueing Sakari Ailus
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).