On 03/27/2012 10:55 PM, Viresh Kumar wrote:
> On 3/27/2012 9:15 PM, Stephen Warren wrote:
>>>> static int __devinit tegra_kbc_probe(struct platform_device *pdev)
>> ...
>>>> + if (pdev->dev.of_node) {
>>>> + /* FIXME: Add handling of linux,fn-keymap here */
>>>> + err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
>>>> + input_dev->keycode, input_dev->keybit,
>>>> + "linux,keymap");
>>
>> Where do input_dev->keycode/keybit get allocated? As far as I can tell,
>> matrix_keypad_of_build_keymap() just writes to those and assumes they're
>> already allocated.
>
> If i am not reading the code incorrectly, keycode is allocated memory with
> kbc. And then we do this:
>
> input_dev->keycode = kbc->keycode;
>
> keybit is again present as part of struct input_dev.
> Am i missing something.
Ah right, I'd been looking inside input_allocate_device() rather than
the specific driver's probe(). So, no issue here.
>>>> diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
>> ...
>>>> +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int
>>>> row_shift,
>> ...
>>>> + keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
>>>> + __set_bit(code, keybit);
>> How bit are keymap and keybit?
>
> Couldn't get this one. :(
> Can you please elaborate the question a bit?
s/bit/big/ might make the question clearer:-)
>> I think we need range-checking here to
>> make sure that row/col/row_shift/code are valid and in-range.
>
> I picked this directly from matrix_keypad_build_keymap() as is.
> Anyway there is no loss in improving it. :)
>
> What kind of range-check you are looking for?
> Currently we do following
>
> unsigned int row = KEY_ROW(key);
> unsigned int col = KEY_COL(key);
> unsigned short code = KEY_VAL(key);
>
> All these macros '&' 'key' with 0xFF, 0xFF and 0xFFFF.
> Which is also kind of range checking.
The size of the keycode array is much smaller though:
mach-tegra's kbc.h:
#define KBC_MAX_ROW 16
#define KBC_MAX_COL 8
#define KBC_MAX_KEY (KBC_MAX_ROW * KBC_MAX_COL)
tegra-kbc.c:
struct tegra_kbc {
...
unsigned short keycode[KBC_MAX_KEY * 2];
so the DT parsing can easily overflow this, and is driven by
user-supplied data. I think you'll need to pass input_dev->keycodesize
into matrix_keypad_of_build_keymap() to achieve the overall
range-checking, but also checking col against (1<<row_shift) would be
needed to make sure the individual "fields" of the array index don't
overflow too.
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
