* [PATCH 1/2] si2168: Bounds check firmware
@ 2015-09-30 0:10 Laura Abbott
2015-09-30 0:10 ` [PATCH 2/2] si2157: " Laura Abbott
0 siblings, 1 reply; 6+ messages in thread
From: Laura Abbott @ 2015-09-30 0:10 UTC (permalink / raw)
To: Antti Palosaari, Mauro Carvalho Chehab
Cc: Laura Abbott, Olli Salonen, linux-media, linux-kernel,
Stuart Auchterlonie, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] si2157: Bounds check firmware
2015-09-30 0:10 [PATCH 1/2] si2168: Bounds check firmware Laura Abbott
@ 2015-09-30 0:10 ` Laura Abbott
2015-10-05 22:24 ` Olli Salonen
0 siblings, 1 reply; 6+ messages in thread
From: Laura Abbott @ 2015-09-30 0:10 UTC (permalink / raw)
To: Antti Palosaari, Mauro Carvalho Chehab
Cc: Laura Abbott, Olli Salonen, linux-media, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] si2157: Bounds check firmware
2015-09-30 0:10 ` [PATCH 2/2] si2157: " Laura Abbott
@ 2015-10-05 22:24 ` Olli Salonen
2015-10-05 22:28 ` Laura Abbott
0 siblings, 1 reply; 6+ messages in thread
From: Olli Salonen @ 2015-10-05 22:24 UTC (permalink / raw)
To: Laura Abbott
Cc: Antti Palosaari, Mauro Carvalho Chehab, linux-media, linux-kernel,
stable
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] si2157: Bounds check firmware
2015-10-05 22:24 ` Olli Salonen
@ 2015-10-05 22:28 ` Laura Abbott
2015-10-05 22:33 ` [PATCHv2] " Laura Abbott
0 siblings, 1 reply; 6+ messages in thread
From: Laura Abbott @ 2015-10-05 22:28 UTC (permalink / raw)
To: Olli Salonen, Laura Abbott
Cc: Antti Palosaari, Mauro Carvalho Chehab, linux-media, linux-kernel,
stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCHv2] si2157: Bounds check firmware
2015-10-05 22:28 ` Laura Abbott
@ 2015-10-05 22:33 ` Laura Abbott
2015-10-08 10:40 ` Olli Salonen
0 siblings, 1 reply; 6+ messages in thread
From: Laura Abbott @ 2015-10-05 22:33 UTC (permalink / raw)
To: Antti Palosaari, Mauro Carvalho Chehab, Olli Salonen
Cc: Laura Abbott, linux-media, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCHv2] si2157: Bounds check firmware
2015-10-05 22:33 ` [PATCHv2] " Laura Abbott
@ 2015-10-08 10:40 ` Olli Salonen
0 siblings, 0 replies; 6+ messages in thread
From: Olli Salonen @ 2015-10-08 10:40 UTC (permalink / raw)
To: Laura Abbott
Cc: Antti Palosaari, Mauro Carvalho Chehab, linux-media, linux-kernel,
stable
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-08 10:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-30 0:10 [PATCH 1/2] si2168: Bounds check firmware Laura Abbott
2015-09-30 0:10 ` [PATCH 2/2] si2157: " Laura Abbott
2015-10-05 22:24 ` Olli Salonen
2015-10-05 22:28 ` Laura Abbott
2015-10-05 22:33 ` [PATCHv2] " Laura Abbott
2015-10-08 10:40 ` Olli Salonen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).