From: Daniel Thompson <daniel.thompson@linaro.org>
To: Colin King <colin.king@canonical.com>
Cc: Lee Jones <lee.jones@linaro.org>,
Jingoo Han <jingoohan1@gmail.com>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Gyungoh Yoo <jack.yoo@skyworksinc.com>,
Bryan Wu <cooloney@gmail.com>,
dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH][V2] backlight: sky81452: insure while loop does not allow negative array indexing
Date: Thu, 27 Feb 2020 11:46:23 +0000 [thread overview]
Message-ID: <20200227114623.vaevrdwiduxa2mqs@holly.lan> (raw)
In-Reply-To: <20200226195826.6567-1-colin.king@canonical.com>
On Wed, Feb 26, 2020 at 07:58:26PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the unlikely event that num_entry is zero, the while loop
> pre-decrements num_entry to cause negative array indexing into the
> array sources. Fix this by iterating only if num_entry >= 0.
>
> Addresses-Coverity: ("Out-of-bounds read")
> Fixes: f705806c9f35 ("backlight: Add support Skyworks SKY81452 backlight driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>
> V2: fix typo in commit subject line
Isn't the correct spelling "ensure"?
> ---
> drivers/video/backlight/sky81452-backlight.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/sky81452-backlight.c b/drivers/video/backlight/sky81452-backlight.c
> index 2355f00f5773..f456930ce78e 100644
> --- a/drivers/video/backlight/sky81452-backlight.c
> +++ b/drivers/video/backlight/sky81452-backlight.c
> @@ -200,7 +200,7 @@ static struct sky81452_bl_platform_data *sky81452_bl_parse_dt(
> }
>
> pdata->enable = 0;
> - while (--num_entry)
> + while (--num_entry >= 0)
> pdata->enable |= (1 << sources[num_entry]);
This look still looks buggy to me (so I'd second Walter's request to
change it to a for loop). If the code genuinely does not contain a
bug then it probably needs a prominent comment explaining why it is
correct not to honour sources[0]!
Daniel.
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Colin King <colin.king@canonical.com>
Cc: linux-fbdev@vger.kernel.org,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
kernel-janitors@vger.kernel.org,
Jingoo Han <jingoohan1@gmail.com>, Bryan Wu <cooloney@gmail.com>,
Gyungoh Yoo <jack.yoo@skyworksinc.com>,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
Lee Jones <lee.jones@linaro.org>
Subject: Re: [PATCH][V2] backlight: sky81452: insure while loop does not allow negative array indexing
Date: Thu, 27 Feb 2020 11:46:23 +0000 [thread overview]
Message-ID: <20200227114623.vaevrdwiduxa2mqs@holly.lan> (raw)
In-Reply-To: <20200226195826.6567-1-colin.king@canonical.com>
On Wed, Feb 26, 2020 at 07:58:26PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the unlikely event that num_entry is zero, the while loop
> pre-decrements num_entry to cause negative array indexing into the
> array sources. Fix this by iterating only if num_entry >= 0.
>
> Addresses-Coverity: ("Out-of-bounds read")
> Fixes: f705806c9f35 ("backlight: Add support Skyworks SKY81452 backlight driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>
> V2: fix typo in commit subject line
Isn't the correct spelling "ensure"?
> ---
> drivers/video/backlight/sky81452-backlight.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/sky81452-backlight.c b/drivers/video/backlight/sky81452-backlight.c
> index 2355f00f5773..f456930ce78e 100644
> --- a/drivers/video/backlight/sky81452-backlight.c
> +++ b/drivers/video/backlight/sky81452-backlight.c
> @@ -200,7 +200,7 @@ static struct sky81452_bl_platform_data *sky81452_bl_parse_dt(
> }
>
> pdata->enable = 0;
> - while (--num_entry)
> + while (--num_entry >= 0)
> pdata->enable |= (1 << sources[num_entry]);
This look still looks buggy to me (so I'd second Walter's request to
change it to a for loop). If the code genuinely does not contain a
bug then it probably needs a prominent comment explaining why it is
correct not to honour sources[0]!
Daniel.
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Colin King <colin.king@canonical.com>
Cc: linux-fbdev@vger.kernel.org,
Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
kernel-janitors@vger.kernel.org,
Jingoo Han <jingoohan1@gmail.com>, Bryan Wu <cooloney@gmail.com>,
Gyungoh Yoo <jack.yoo@skyworksinc.com>,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
Lee Jones <lee.jones@linaro.org>
Subject: Re: [PATCH][V2] backlight: sky81452: insure while loop does not allow negative array indexing
Date: Thu, 27 Feb 2020 11:46:23 +0000 [thread overview]
Message-ID: <20200227114623.vaevrdwiduxa2mqs@holly.lan> (raw)
In-Reply-To: <20200226195826.6567-1-colin.king@canonical.com>
On Wed, Feb 26, 2020 at 07:58:26PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the unlikely event that num_entry is zero, the while loop
> pre-decrements num_entry to cause negative array indexing into the
> array sources. Fix this by iterating only if num_entry >= 0.
>
> Addresses-Coverity: ("Out-of-bounds read")
> Fixes: f705806c9f35 ("backlight: Add support Skyworks SKY81452 backlight driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>
> V2: fix typo in commit subject line
Isn't the correct spelling "ensure"?
> ---
> drivers/video/backlight/sky81452-backlight.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/sky81452-backlight.c b/drivers/video/backlight/sky81452-backlight.c
> index 2355f00f5773..f456930ce78e 100644
> --- a/drivers/video/backlight/sky81452-backlight.c
> +++ b/drivers/video/backlight/sky81452-backlight.c
> @@ -200,7 +200,7 @@ static struct sky81452_bl_platform_data *sky81452_bl_parse_dt(
> }
>
> pdata->enable = 0;
> - while (--num_entry)
> + while (--num_entry >= 0)
> pdata->enable |= (1 << sources[num_entry]);
This look still looks buggy to me (so I'd second Walter's request to
change it to a for loop). If the code genuinely does not contain a
bug then it probably needs a prominent comment explaining why it is
correct not to honour sources[0]!
Daniel.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-02-27 11:46 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-26 19:58 [PATCH][V2] backlight: sky81452: insure while loop does not allow negative array indexing Colin King
2020-02-26 19:58 ` Colin King
2020-02-27 11:15 ` AW: " Walter Harms
2020-02-27 11:15 ` Walter Harms
2020-02-27 11:46 ` Daniel Thompson [this message]
2020-02-27 11:46 ` Daniel Thompson
2020-02-27 11:46 ` Daniel Thompson
2020-02-27 14:57 ` Daniel Thompson
2020-02-27 14:57 ` Daniel Thompson
2020-02-27 14:57 ` Daniel Thompson
2020-02-27 15:10 ` Colin Ian King
2020-02-27 15:10 ` Colin Ian King
2020-02-27 15:10 ` Colin Ian King
2020-02-28 16:45 ` Daniel Thompson
2020-02-28 16:45 ` Daniel Thompson
2020-02-28 16:45 ` Daniel Thompson
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=20200227114623.vaevrdwiduxa2mqs@holly.lan \
--to=daniel.thompson@linaro.org \
--cc=b.zolnierkie@samsung.com \
--cc=colin.king@canonical.com \
--cc=cooloney@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=jack.yoo@skyworksinc.com \
--cc=jingoohan1@gmail.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.