Fix NR_KEYS off-by-one error

KDGKBENT ioctl can use 256 entries (0-255), but it was defined as key_map[NR_KEYS] (NR_KEYS == 255). The code seems also thinking it's 256.

	key_map[0] = U(K_ALLOCATED);
	for (j = 1; j < NR_KEYS; j++)
		key_map[j] = U(K_HOLE);

Signed-off-by OGAWA Hirofumi <[email protected]>

---

 include/linux/keyboard.h |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN include/linux/keyboard.h~nr_keys-off-by-one include/linux/keyboard.h

include/linux/keyboard.h renamed from include/linux/keyboard.h~nr_keys-off-by-one +1 -1

@@ -16,7 +16,7 @@
 
 #define NR_SHIFT	9
 
-#define NR_KEYS		255
+#define NR_KEYS		256
 #define MAX_NR_KEYMAPS	256
 /* This means 128Kb if all keymaps are allocated. Only the superuser
 	may increase the number of keymaps beyond MAX_NR_OF_USER_KEYMAPS. */

_

Quoted 6 lines · OGAWA Hirofumi

KDGKBENT ioctl can use 256 entries (0-255), but it was defined as key_map[NR_KEYS] (NR_KEYS == 255). The code seems also thinking it's 256.

	key_map[0] = U(K_ALLOCATED);
	for (j = 1; j < NR_KEYS; j++)
		key_map[j] = U(K_HOLE);

The patch below might cause problems, though, because some apps may (in old versions are) using a char variable to index up to NR_KEYS, which leads to an endless loop.

The patch might cause problems, though, because some apps may (in old versions are) using a char variable to index up to NR_KEYS, which leads to an endless loop.

On Sat, Jul 17, 2004 at 01:35:59AM +0900, OGAWA Hirofumi wrote:

:: KDGKBENT ioctl can use 256 entries (0-255), but it was defined as :: key_map[NR_KEYS] (NR_KEYS == 255). The code seems also thinking it's 256. ::
:: key_map[0] = U(K_ALLOCATED);
:: for (j = 1; j < NR_KEYS; j++)
:: key_map[j] = U(K_HOLE);

I think the code has no opinion. It was 128 in 2.4. I am not aware of assumptions on NR_KEYS. So, do not think this is an off-by-one error.

Quoted 9 lines · Andries Brouwer

On Sat, Jul 17, 2004 at 01:35:59AM +0900, OGAWA Hirofumi wrote:

:: KDGKBENT ioctl can use 256 entries (0-255), but it was defined as :: key_map[NR_KEYS] (NR_KEYS == 255). The code seems also thinking it's 256. ::
:: key_map[0] = U(K_ALLOCATED);
:: for (j = 1; j < NR_KEYS; j++)
:: key_map[j] = U(K_HOLE);

Quoted 3 lines

I think the code has no opinion. It was 128 in 2.4. I am not aware of assumptions on NR_KEYS. So, do not think this is an off-by-one error.

My point is that key_map is 0-254 array. But KDGKBENT uses 255

	case KDGKBENT:
		key_map = key_maps[s];
		if (key_map) {
		    val = U(key_map[i]);
		    if (kbd->kbdmode != VC_UNICODE && KTYP(val) >= NR_TYPES)
			val = K_HOLE;
		} else
		    val = (i ? K_HOLE : K_NOSUCHMAP);
		return put_user(val, &user_kbe->kb_value);

Quoted 18 lines · OGAWA Hirofumi

Quoted 9 lines · Andries Brouwer

On Sat, Jul 17, 2004 at 01:35:59AM +0900, OGAWA Hirofumi wrote:

:: KDGKBENT ioctl can use 256 entries (0-255), but it was defined as :: key_map[NR_KEYS] (NR_KEYS == 255). The code seems also thinking it's 256. ::
:: key_map[0] = U(K_ALLOCATED);
:: for (j = 1; j < NR_KEYS; j++)
:: key_map[j] = U(K_HOLE);

Quoted 3 lines

I think the code has no opinion. It was 128 in 2.4. I am not aware of assumptions on NR_KEYS. So, do not think this is an off-by-one error.

My point is that key_map is 0-254 array. But KDGKBENT uses 255

Quoted 10 lines

	case KDGKBENT:
		key_map = key_maps[s];
		if (key_map) {
		    val = U(key_map[i]);
		    if (kbd->kbdmode != VC_UNICODE && KTYP(val) >= NR_TYPES)
			val = K_HOLE;
		} else
		    val = (i ? K_HOLE : K_NOSUCHMAP);
		return put_user(val, &user_kbe->kb_value);

This all seems a bit inconclusive. Do we proceed with the original patch or not? If not, how do we fix the overflow which Hirofumi has identified?

Quoted 2 lines

This all seems a bit inconclusive. Do we proceed with the original patch or not? If not, how do we fix the overflow which Hirofumi has identified?

I think we should check the value in the ioctl, regardless of what's NR_KEYS defined to.

Quoted 5 lines · Vojtech Pavlik

Quoted 2 lines

This all seems a bit inconclusive. Do we proceed with the original patch or not? If not, how do we fix the overflow which Hirofumi has identified?

I think we should check the value in the ioctl, regardless of what's NR_KEYS defined to.

However, it breaks the current binary instead. (at least console-tools, kbdutils).

Quoted 5 lines · OGAWA Hirofumi

Quoted 3 lines · Vojtech Pavlik

Quoted 2 lines

This all seems a bit inconclusive. Do we proceed with the original patch or not? If not, how do we fix the overflow which Hirofumi has identified?

Quoted 5 lines

Quoted 2 lines

I think we should check the value in the ioctl, regardless of what's NR_KEYS defined to.

However, it breaks the current binary instead. (at least console-tools, kbdutils).

We can do both, then, if that helps.

Quoted 10 lines

:: KDGKBENT ioctl can use 256 entries (0-255), but it was defined as :: key_map[NR_KEYS] (NR_KEYS == 255). The code seems also thinking it's 256. ::
:: key_map[0] = U(K_ALLOCATED);
:: for (j = 1; j < NR_KEYS; j++)
:: key_map[j] = U(K_HOLE);

I think the code has no opinion. It was 128 in 2.4. I am not aware of assumptions on NR_KEYS. So, do not think this is an off-by-one error.

My point is that key_map is 0-254 array. But KDGKBENT uses 255

	case KDGKBENT:
		key_map = key_maps[s];
		if (key_map) {
		    val = U(key_map[i]);

When an array has an arbitrary upper bound that can be changed via a #define, and for some values of the upper bound a test is superfluous, that does not mean that the test is superfluous.

Any comments or suggestions?

drivers/char/vt_ioctl.c +0 -3

-#define i (tmp.kb_index)
-#define s (tmp.kb_table)
-#define v (tmp.kb_value)
 static inline int
 do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm, struct kbd_struct *kbd)
 {
 	struct kbentry tmp;

because it limits the range when struct kbentry
was changed. (I think this change is likely)

key_map is pointer, so ARRAY_SIZE() can't use.

Anyhow these will be warned by gcc.

Although overhead is insignificance, I'd hated to add overhead of this test because test is not needed right now.

drivers/char/vt_ioctl.c unsigned char keyboard_type = KB_101; · line 80

Quoted 2 lines

drivers/char/vt_ioctl.c +2 -0

+	unsigned char s, i;
+	unsigned short v;

would limit s, i, and v if struct kbentry was changed to have larger types (short or int, say) for these.

Good point.

Would it work to use larger types for local variables s, i, and v now, in order to avoid the ugly macro, as in:

	unsigned int s, i, v;

Quoted 1 line

Anyhow these will be warned by gcc.

If larger types, as I wrote above, were used for s, i, and v, then does gcc still warn?

Quoted 2 lines

Although overhead is insignificance, I'd hated to add overhead of this test because test is not needed right now.

Code clarity matters most here. If the code had been crystal clear to the casual reader, then the initial mistake, of removing the range checking, probably would never have occurred in the first place, and we human beings would have already saved more time than we can ever hope to save by optimizing this code.

You are absolutely correct that overhead is insignificant.

But code clarity - that is very significant <smile>.

Let all the essential details be spelled out, in the simplest most easily read, C statements that can be found to express it.

Each line of code we put in the kernel will be read by many people doing various things. They will be more likely to have a correct understanding of our code if it is clear and simple, with a minimum of surprises.

I didn't see any big cleanup in your patch.

... still waiting ...

Don't wait on me ... as indicated in my last post on this lkml thread, I left this back in the hands of the expert, OGAWA Hirofumi.

Hate to be a bore, but I'm still waiting for a definitive patch ;)

Could you please post your lastest patch? It seems that my patch does not satisfy peoples.

Let me summarize.

In the past, the kernel had various different values of NR_KEYS, in this
order: 128, 512, 256, 255.

128 was not enough, 512 didn't fit in a byte (while allowed to address all keycodes the input layer uses), 256 broke some apps that relied on unsigned char counters,

...
BUT some binaries are still compiled with 256 and try to set up a mapping for keycode 255 (although there is _no_ such keycode), and break. IMO it's a bug in the app.

Now I believe that simply adding the check back by reverting the old Andrew's patch and recompiling/fixing what breaks is the right way to go.

Quoted 8 lines

Let me summarize.

In the past, the kernel had various different values of NR_KEYS, in this
order: 128, 512, 256, 255.

128 was not enough, 512 didn't fit in a byte (while allowed to address all keycodes the input layer uses), 256 broke some apps that relied on unsigned char counters,

Can you elaborate on this part? Which applications broke?

Quoted 7 lines

BUT some binaries are still compiled with 256 and try to set up a mapping for keycode 255 (although there is _no_ such keycode), and break. IMO it's a bug in the app.

Now I believe that simply adding the check back by reverting the old Andrew's patch and recompiling/fixing what breaks is the right way to go.

Revert Andrew's patch: yes.
Choosing 255/256 - I have no opinion yet, my opinion will depend on your answer to the above "Which applications broke?".

Quoted 2 lines · Vojtech Pavlik

Let me summarize.

Quoted 6 lines

In the past, the kernel had various different values of NR_KEYS, in this
order: 128, 512, 256, 255.

128 was not enough, 512 didn't fit in a byte (while allowed to address all keycodes the input layer uses), 256 broke some apps that relied on unsigned char counters,

Can you elaborate on this part? Which applications broke?

Revert Andrew's patch: yes.
Choosing 255/256 - I have no opinion yet, my opinion will depend on your answer to the above "Which applications broke?".