Re: [PATCH 2/2] si2157: Bounds check firmware

[Olli Salonen]

Hi Laura,

While the patch itself does what it says, the return code for the
si2157_init will be 0 even if there's a faulty firmware file. Wouldn't
it be better to set the return code as -EINVAL like done a few lines
earlier in the code (see below)?

if (fw->size % 17 != 0) {
dev_err(>dev, "firmware file '%s' is invalid\n",
fw_name);
ret = -EINVAL;
goto err_release_firmware;
}

Cheers,
-olli

On 30 September 2015 at 02:10, Laura Abbott  wrote:
> When reading the firmware and sending commands, the length
> must be bounds checked to avoid overrunning the size of the command
> buffer and smashing the stack if the firmware is not in the
> expected format. Add the proper check.
>
> Cc: sta...@kernel.org
> Signed-off-by: Laura Abbott 
> ---
>  drivers/media/tuners/si2157.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> index 5073821..ce157ed 100644
> --- a/drivers/media/tuners/si2157.c
> +++ b/drivers/media/tuners/si2157.c
> @@ -166,6 +166,10 @@ static int si2157_init(struct dvb_frontend *fe)
>
> for (remaining = fw->size; remaining > 0; remaining -= 17) {
> len = fw->data[fw->size - remaining];
> +   if (len > SI2157_ARGLEN) {
> +   dev_err(>dev, "Bad firmware length\n");
> +   goto err_release_firmware;
> +   }
> memcpy(cmd.args, >data[(fw->size - remaining) + 1], len);
> cmd.wlen = len;
> cmd.rlen = 1;
> --
> 2.4.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] si2157: Bounds check firmware

[Laura Abbott]

On 10/05/2015 03:24 PM, Olli Salonen wrote:

Hi Laura,

While the patch itself does what it says, the return code for the
si2157_init will be 0 even if there's a faulty firmware file. Wouldn't
it be better to set the return code as -EINVAL like done a few lines
earlier in the code (see below)?

 if (fw->size % 17 != 0) {
 dev_err(>dev, "firmware file '%s' is invalid\n",
 fw_name);
 ret = -EINVAL;
 goto err_release_firmware;
 }

Cheers,
-olli




Right, I'll update with v2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] si2157: Bounds check firmware

[Laura Abbott]

When reading the firmware and sending commands, the length
must be bounds checked to avoid overrunning the size of the command
buffer and smashing the stack if the firmware is not in the
expected format. Add the proper check.

Cc: sta...@kernel.org
Signed-off-by: Laura Abbott 
---
 drivers/media/tuners/si2157.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 5073821..ce157ed 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -166,6 +166,10 @@ static int si2157_init(struct dvb_frontend *fe)
 
for (remaining = fw->size; remaining > 0; remaining -= 17) {
len = fw->data[fw->size - remaining];
+   if (len > SI2157_ARGLEN) {
+   dev_err(>dev, "Bad firmware length\n");
+   goto err_release_firmware;
+   }
memcpy(cmd.args, >data[(fw->size - remaining) + 1], len);
cmd.wlen = len;
cmd.rlen = 1;
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html