re: mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

re: mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Colin Ian King]

Hi there,

Static analysis with CoverityScan reported a potential issue with the
following commit:

commit 0f7b126ca91101d02d525f7cc880e8c71202a2b7
Author: Stefan Agner <stefan@agner.ch>
Date:   Sun Jun 24 23:27:25 2018 +0200

    mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver


in function tegra_nand_cmd it looks like there maybe potential to pass a
negative value in size into memcpy():

        case NAND_OP_DATA_OUT_INSTR:

negative_return_fn: Function nand_subop_get_data_len(subop, op_id)
returns a negative number.

var_assign: Assigning: unsigned variable size = nand_subop_get_data_len.

                size = nand_subop_get_data_len(subop, op_id);
                offset = nand_subop_get_data_start_off(subop, op_id);

                cmd |= COMMAND_TRANS_SIZE(size) | COMMAND_PIO |
                        COMMAND_TX | COMMAND_A_VALID;

Argument cannot be negative (NEGATIVE_RETURNS)65. negative_returns: size
is passed to a parameter that cannot be negative.
                memcpy(&reg, instr->ctx.data.buf.out + offset, size);


I also think the same issue may be happening with the
NAND_OP_DATA_IN_INSTR case too.

Colin

Re: mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Miquel Raynal]

Hi Colin, Stefan,

+linux-mtd

Thanks Colin for the report.

On Tue, 26 Jun 2018 16:18:29 +0100, Colin Ian King
<colin.king@canonical.com> wrote:

> Hi there,
> 
> Static analysis with CoverityScan reported a potential issue with the
> following commit:
> 
> commit 0f7b126ca91101d02d525f7cc880e8c71202a2b7
> Author: Stefan Agner <stefan@agner.ch>
> Date:   Sun Jun 24 23:27:25 2018 +0200
> 
>     mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
> 
> 
> in function tegra_nand_cmd it looks like there maybe potential to pass a
> negative value in size into memcpy():
> 
>         case NAND_OP_DATA_OUT_INSTR:
> 
> negative_return_fn: Function nand_subop_get_data_len(subop, op_id)
> returns a negative number.
> 
> var_assign: Assigning: unsigned variable size = nand_subop_get_data_len.
> 
>                 size = nand_subop_get_data_len(subop, op_id);
>                 offset = nand_subop_get_data_start_off(subop, op_id);

Stefan,

I thought a bit about this and I don't think the right place for such a
fix are the NAND controller drivers (marvell and vf610 have the same
issue). Both nand_subop_get_data/addr_len/start_off() are core helpers
and their result is predictable in a manner that only a bug in your
parsing function would trigger an error value. I think this is safe for
the four helpers to have WARN_ON() on the error conditions to catch
the developer's attention and just return (unsigned int) 0 in this case.

I will propose something soon.

Thanks,
Miquèl
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html