Re: [PATCH V2 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Stephen Warren <swarren@wwwdotorg.org>]

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.