[PATCH 1/2] si2168: Bounds check firmware

[PATCH 1/2] si2168: 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:

si2168 11-0064: found a 'Silicon Labs Si2168-B40'
si2168 11-0064: downloading firmware from file 'dvb-demod-si2168-b40-01.fw'
si2168 11-0064: firmware download failed -95
Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffffa085708f

Add the proper check.

Cc: stable@kernel.org
Reported-by: Stuart Auchterlonie <sauchter@redhat.com>
Reviewed-by: Antti Palosaari <crope@iki.fi>
Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
---
 drivers/media/dvb-frontends/si2168.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
index 81788c5..821a8f4 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -502,6 +502,10 @@ static int si2168_init(struct dvb_frontend *fe)
 		/* firmware is in the new format */
 		for (remaining = fw->size; remaining > 0; remaining -= 17) {
 			len = fw->data[fw->size - remaining];
+			if (len > SI2168_ARGLEN) {
+				ret = -EINVAL;
+				break;
+			}
 			memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
 			cmd.wlen = len;
 			cmd.rlen = 1;
-- 
2.4.3

[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: stable@kernel.org
Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
---
 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(&client->dev, "Bad firmware length\n");
+			goto err_release_firmware;
+		}
 		memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
 		cmd.wlen = len;
 		cmd.rlen = 1;
-- 
2.4.3

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(&client->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 <labbott@fedoraproject.org> 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: stable@kernel.org
> Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
> ---
>  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(&client->dev, "Bad firmware length\n");
> +                       goto err_release_firmware;
> +               }
>                 memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
>                 cmd.wlen = len;
>                 cmd.rlen = 1;
> --
> 2.4.3
>

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(&client->dev, "firmware file '%s' is invalid\n",
>                                  fw_name);
>                  ret = -EINVAL;
>                  goto err_release_firmware;
>          }
>
> Cheers,
> -olli
>


Right, I'll update with v2

[PATCHv2] 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: stable@kernel.org
Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
---
v2: Set the return code properly
---
 drivers/media/tuners/si2157.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 5073821..0e1ca2b 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -166,6 +166,11 @@ 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(&client->dev, "Bad firmware length\n");
+			ret = -EINVAL;
+			goto err_release_firmware;
+		}
 		memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
 		cmd.wlen = len;
 		cmd.rlen = 1;
-- 
2.4.3

Re: [PATCHv2] si2157: Bounds check firmware

[Olli Salonen]

Reviewed-by: Olli Salonen <olli.salonen@iki.fi>
Tested-by: Olli Salonen <olli.salonen@iki.fi>


On 6 October 2015 at 01:33, Laura Abbott <labbott@fedoraproject.org> 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: stable@kernel.org
> Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
> ---
> v2: Set the return code properly
> ---
>  drivers/media/tuners/si2157.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
> index 5073821..0e1ca2b 100644
> --- a/drivers/media/tuners/si2157.c
> +++ b/drivers/media/tuners/si2157.c
> @@ -166,6 +166,11 @@ 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(&client->dev, "Bad firmware length\n");
> +                       ret = -EINVAL;
> +                       goto err_release_firmware;
> +               }
>                 memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
>                 cmd.wlen = len;
>                 cmd.rlen = 1;
> --
> 2.4.3
>