diff mbox

[media] media: info leak in media_device_enum_entities()

Message ID 20130421111003.GD6171@elgon.mountain (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter April 21, 2013, 11:10 a.m. UTC
The last part of the "u_ent.name" buffer isn't cleared so it still has
uninitialized stack memory.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

--
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

Comments

Walter Harms April 21, 2013, 11:51 a.m. UTC | #1
Am 21.04.2013 13:10, schrieb Dan Carpenter:
> The last part of the "u_ent.name" buffer isn't cleared so it still has
> uninitialized stack memory.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 99b80b6..1957c0d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -102,9 +102,12 @@ static long media_device_enum_entities(struct media_device *mdev,
>  		return -EINVAL;
>  
>  	u_ent.id = ent->id;
> -	u_ent.name[0] = '\0';
> -	if (ent->name)
> -		strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
> +	if (ent->name) {
> +		strncpy(u_ent.name, ent->name, sizeof(u_ent.name));
> +		u_ent.name[sizeof(u_ent.name) - 1] = '\0';
> +	} else {
> +		memset(u_ent.name, 0, sizeof(u_ent.name));
> +	}

I would always memset()
and then do strncpy() for sizeof(u_ent.name) - 1
the rest is always zero.

re,
 wh


>  	u_ent.type = ent->type;
>  	u_ent.revision = ent->revision;
>  	u_ent.flags = ent->flags;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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
Dan Carpenter April 21, 2013, 4:42 p.m. UTC | #2
On Sun, Apr 21, 2013 at 01:51:56PM +0200, walter harms wrote:
> 
> 
> Am 21.04.2013 13:10, schrieb Dan Carpenter:
> > The last part of the "u_ent.name" buffer isn't cleared so it still has
> > uninitialized stack memory.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 99b80b6..1957c0d 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -102,9 +102,12 @@ static long media_device_enum_entities(struct media_device *mdev,
> >  		return -EINVAL;
> >  
> >  	u_ent.id = ent->id;
> > -	u_ent.name[0] = '\0';
> > -	if (ent->name)
> > -		strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
> > +	if (ent->name) {
> > +		strncpy(u_ent.name, ent->name, sizeof(u_ent.name));
> > +		u_ent.name[sizeof(u_ent.name) - 1] = '\0';
> > +	} else {
> > +		memset(u_ent.name, 0, sizeof(u_ent.name));
> > +	}
> 
> I would always memset()
> and then do strncpy() for sizeof(u_ent.name) - 1
> the rest is always zero.

Both ways are fine.  You'd still have to test for "if (ent->name)",
of course.  This way is a little faster because I do the test first.

Mauro, if you want I can redo it?

regards,
dan carpenter

--
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 mbox

Patch

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 99b80b6..1957c0d 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -102,9 +102,12 @@  static long media_device_enum_entities(struct media_device *mdev,
 		return -EINVAL;
 
 	u_ent.id = ent->id;
-	u_ent.name[0] = '\0';
-	if (ent->name)
-		strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
+	if (ent->name) {
+		strncpy(u_ent.name, ent->name, sizeof(u_ent.name));
+		u_ent.name[sizeof(u_ent.name) - 1] = '\0';
+	} else {
+		memset(u_ent.name, 0, sizeof(u_ent.name));
+	}
 	u_ent.type = ent->type;
 	u_ent.revision = ent->revision;
 	u_ent.flags = ent->flags;