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

[Michael Ira Krufky]

On Sat, Nov 29, 2014 at 10:04 PM, Nicholas Krause <xerofoify@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/