Message ID | 1432139980-12619-14-git-send-email-william.towle@codethink.co.uk (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Guennadi Liakhovetski |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1YvMNk-0002iC-Ut; Thu, 21 May 2015 11:03:12 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-8) with esmtp id 1YvMNj-0006LD-jH; Thu, 21 May 2015 11:03:12 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932167AbbEUJDF (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 21 May 2015 05:03:05 -0400 Received: from 82-70-136-246.dsl.in-addr.zen.co.uk ([82.70.136.246]:56474 "EHLO xk120.dyn.ducie.codethink.co.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753677AbbEUJDD (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 21 May 2015 05:03:03 -0400 Received: from william by xk120.dyn.ducie.codethink.co.uk with local (Exim 4.80) (envelope-from <william.towle@codethink.co.uk>) id 1Yv72H-0004CN-Ns; Wed, 20 May 2015 17:40:01 +0100 From: William Towle <william.towle@codethink.co.uk> To: linux-kernel@lists.codethink.co.uk, linux-media@vger.kernel.org Cc: g.liakhovetski@gmx.de, sergei.shtylyov@cogentembedded.com, hverkuil@xs4all.nl, rob.taylor@codethink.co.uk Subject: [PATCH 13/20] media: soc_camera: v4l2-compliance fixes for querycap Date: Wed, 20 May 2015 17:39:33 +0100 Message-Id: <1432139980-12619-14-git-send-email-william.towle@codethink.co.uk> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1432139980-12619-1-git-send-email-william.towle@codethink.co.uk> References: <1432139980-12619-1-git-send-email-william.towle@codethink.co.uk> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2015.5.21.85416 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1000_1099 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, REFERENCES 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __REFERENCES 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
William Towle
May 20, 2015, 4:39 p.m. UTC
Fill in bus_info field and zero reserved field. Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk> Reviewed-by: William Towle <william.towle@codethink.co.uk> --- drivers/media/platform/soc_camera/soc_camera.c | 2 ++ 1 file changed, 2 insertions(+)
Comments
On 21/05/15 06:58, Hans Verkuil wrote: > On 05/20/2015 06:39 PM, William Towle wrote: >> Fill in bus_info field and zero reserved field. >> >> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk> >> Reviewed-by: William Towle <william.towle@codethink.co.uk> >> --- >> drivers/media/platform/soc_camera/soc_camera.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c >> index fd7497e..583c5e6 100644 >> --- a/drivers/media/platform/soc_camera/soc_camera.c >> +++ b/drivers/media/platform/soc_camera/soc_camera.c >> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, >> WARN_ON(priv != file->private_data); >> >> strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver)); >> + strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info)); >> + memset(cap->reserved, 0, sizeof(cap->reserved)); > > Why the memset? That shouldn't be needed. v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in v4l-utils v1.6.2 [1]) Thanks, Rob [1] http://git.linuxtv.org/cgit.cgi/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-compliance.cpp?id=v4l-utils-1.6.2#n308 > Regards, > > Hans > >> return ici->ops->querycap(ici, cap); >> } >> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi William, On Wed, 20 May 2015, William Towle wrote: > Fill in bus_info field and zero reserved field. > > Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk> If you're the author and the submitter of this patch, why is Rob's Sob here needed? Or is he the author? Thanks Guennadi > Reviewed-by: William Towle <william.towle@codethink.co.uk> > --- > drivers/media/platform/soc_camera/soc_camera.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c > index fd7497e..583c5e6 100644 > --- a/drivers/media/platform/soc_camera/soc_camera.c > +++ b/drivers/media/platform/soc_camera/soc_camera.c > @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, > WARN_ON(priv != file->private_data); > > strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver)); > + strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info)); > + memset(cap->reserved, 0, sizeof(cap->reserved)); > return ici->ops->querycap(ici, cap); > } > > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote: > On 21/05/15 06:58, Hans Verkuil wrote: > > On 05/20/2015 06:39 PM, William Towle wrote: > >> Fill in bus_info field and zero reserved field. > >> > >> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk> > >> Reviewed-by: William Towle <william.towle@codethink.co.uk> > >> --- > >> drivers/media/platform/soc_camera/soc_camera.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c > >> index fd7497e..583c5e6 100644 > >> --- a/drivers/media/platform/soc_camera/soc_camera.c > >> +++ b/drivers/media/platform/soc_camera/soc_camera.c > >> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, > >> WARN_ON(priv != file->private_data); > >> > >> strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver)); > >> + strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info)); > >> + memset(cap->reserved, 0, sizeof(cap->reserved)); > > > > Why the memset? That shouldn't be needed. > > v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in > v4l-utils v1.6.2 [1]) I'm puzzled by that. Isn't this function called by v4l_querycap(), which is called by video_usercopy()? And video_usercopy() zeroes the entire structure before doing so, or at least it appears to be intended to. Anyway, if we're failing to initialise kernel memory that's copied to user-space, that's a (usually minor) security issue and the fix ought to be cc'd to stable. Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/29/2015 03:08 AM, Ben Hutchings wrote: > On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote: >> On 21/05/15 06:58, Hans Verkuil wrote: >>> On 05/20/2015 06:39 PM, William Towle wrote: >>>> Fill in bus_info field and zero reserved field. >>>> >>>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk> >>>> Reviewed-by: William Towle <william.towle@codethink.co.uk> >>>> --- >>>> drivers/media/platform/soc_camera/soc_camera.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c >>>> index fd7497e..583c5e6 100644 >>>> --- a/drivers/media/platform/soc_camera/soc_camera.c >>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c >>>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, >>>> WARN_ON(priv != file->private_data); >>>> >>>> strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver)); >>>> + strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info)); >>>> + memset(cap->reserved, 0, sizeof(cap->reserved)); >>> >>> Why the memset? That shouldn't be needed. >> >> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in >> v4l-utils v1.6.2 [1]) William, you should use the latest v4l-utils compiled from the git repo. Unlikely to be related to this, though. > > I'm puzzled by that. Isn't this function called by v4l_querycap(), > which is called by video_usercopy()? And video_usercopy() zeroes the > entire structure before doing so, or at least it appears to be intended > to. Right. So I don't understand this. Can you dig a bit deeper why this would be needed here? It should not be necessary at all, so if reserved is non-zero, then someone is writing data where it shouldn't. Regards, Hans > > Anyway, if we're failing to initialise kernel memory that's copied to > user-space, that's a (usually minor) security issue and the fix ought to > be cc'd to stable. > > Ben. > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/05/15 11:10, Hans Verkuil wrote: > On 05/29/2015 03:08 AM, Ben Hutchings wrote: >> On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote: >>> On 21/05/15 06:58, Hans Verkuil wrote: >>>> On 05/20/2015 06:39 PM, William Towle wrote: >>>>> Fill in bus_info field and zero reserved field. >>>>> >>>>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk> >>>>> Reviewed-by: William Towle <william.towle@codethink.co.uk> >>>>> --- >>>>> drivers/media/platform/soc_camera/soc_camera.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c >>>>> index fd7497e..583c5e6 100644 >>>>> --- a/drivers/media/platform/soc_camera/soc_camera.c >>>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c >>>>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, >>>>> WARN_ON(priv != file->private_data); >>>>> >>>>> strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver)); >>>>> + strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info)); >>>>> + memset(cap->reserved, 0, sizeof(cap->reserved)); >>>> >>>> Why the memset? That shouldn't be needed. >>> >>> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in >>> v4l-utils v1.6.2 [1]) > > William, you should use the latest v4l-utils compiled from the git repo. > Unlikely to be related to this, though. > >> >> I'm puzzled by that. Isn't this function called by v4l_querycap(), >> which is called by video_usercopy()? And video_usercopy() zeroes the >> entire structure before doing so, or at least it appears to be intended >> to. > > Right. So I don't understand this. Can you dig a bit deeper why this would > be needed here? It should not be necessary at all, so if reserved is non-zero, > then someone is writing data where it shouldn't. I've just done some digging (including rolling back to when I saw the issue). Turns out the message I was attempting to 'fix' here was from an ancient v4l2-compliance, and clearly the outcome of a late night hacking... The memset is indeed not needed. Thanks Rob -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c index fd7497e..583c5e6 100644 --- a/drivers/media/platform/soc_camera/soc_camera.c +++ b/drivers/media/platform/soc_camera/soc_camera.c @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void *priv, WARN_ON(priv != file->private_data); strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver)); + strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info)); + memset(cap->reserved, 0, sizeof(cap->reserved)); return ici->ops->querycap(ici, cap); }