linux-kernel - Re: [PATCH] drivers:media: Add proper sanity checking for register setting to variable in lg2160.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAOcJUbyjmUGOiAAQTc=O9OMk7vw8A6sYQg9svKNVw_zCyt5Jcg@mail.gmail.com>
Date:	Sun, 30 Nov 2014 11:32:21 -0500
From:	Michael Ira Krufky <mkrufky@...uxtv.org>
To:	Nicholas Krause <xerofoify@...il.com>
Cc:	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	linux-media <linux-media@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drivers:media: Add proper sanity checking for register
 setting to variable in lg2160.c

On Sat, Nov 29, 2014 at 10:04 PM, Nicholas Krause <xerofoify@...il.com> wrote:
> Fixs issue with setting the variable value of val in the functionm,lg2161_set_output_interface
> and not sanity checking if the value has been correctly set with the correct value of the struct
> state of type lg216x_state passed to the function by the calling code.
>
> Signed-off-by: Nicholas Krause <xerofoify@...il.com>
> ---
>  drivers/media/dvb-frontends/lg2160.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/dvb-frontends/lg2160.c b/drivers/media/dvb-frontends/lg2160.c
> index 5fd14f8..9aec63a 100644
> --- a/drivers/media/dvb-frontends/lg2160.c
> +++ b/drivers/media/dvb-frontends/lg2160.c
> @@ -459,7 +459,9 @@ static int lg2161_set_output_interface(struct lg216x_state *state)
>                 goto fail;
>
>         val &= ~0x07;
> -       val |= state->cfg->output_if; /* FIXME: needs sanity check */
> +       val |= state->cfg->output_if;
> +       if (lg_fail(val))
> +               goto fail;
>
>         ret = lg216x_write_reg(state, 0x0014, val);
>         lg_fail(ret);
> --
> 2.1.0
>

NACK.

Thank you for the patch, Nicholas, but the change that you propose
won't actually help here -- val is a u8, and the lg_fail() macro tests
for negative values.  val can never be negative, and that's why I
didn't add the "lg_fail()" test myself when I wrote this function.

What's actually happening in this function is that the value is first
set when reading register 0014, we take that value, unset the three
lowest bits, then we raise the bits in state->cfg->output_if, and
write it back to the same register.

In fact, the FIXME comment should be removed -- this is not a bug
anymore and the sanity check is built-in.  That FIXME comment is a
remnant of an older version of this function :-P

If you want to send a patch that just removes the FIXME comment, that
would be accepted, but this patch should not be merged.

Cheers,

Michael Ira Krufky
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/