[PATCH] Fix NR_KEYS off-by-one error

[PATCH] Fix NR_KEYS off-by-one error

[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);

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 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
--- linux-2.6.8-rc1/include/linux/keyboard.h~nr_keys-off-by-one	2004-07-16 06:20:10.000000000 +0900
+++ linux-2.6.8-rc1-hirofumi/include/linux/keyboard.h	2004-07-16 06:20:10.000000000 +0900
@@ -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. */
_

Re: [PATCH] Fix NR_KEYS off-by-one error

[Vojtech Pavlik]

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);

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.

> 
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
> 
>  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
> --- linux-2.6.8-rc1/include/linux/keyboard.h~nr_keys-off-by-one	2004-07-16 06:20:10.000000000 +0900
> +++ linux-2.6.8-rc1-hirofumi/include/linux/keyboard.h	2004-07-16 06:20:10.000000000 +0900
> @@ -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. */
> _
> 

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Vojtech Pavlik <vojtech@suse.cz> writes:

> 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);
> 
> 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.

Maybe. But isn't it just bug of apps?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Fix NR_KEYS off-by-one error

[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);

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.
Vojtech did this intentionally.

However, I have no objections. In fact loadkeys uses 256.

On Fri, Jul 16, 2004 at 06:44:35PM +0200, Vojtech Pavlik wrote:

> 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.

The binaries will still work. If the utility is recompiled, against
recent kernel headers, then the person doing that is responsible
for the result.

I just checked kbd, but there are no such char variables there.
Do you have specific utilities in mind? Is X OK?

Andries

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Andries Brouwer <aebr@win.tue.nl> writes:

> 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.

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);

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Fix NR_KEYS off-by-one error

[Andrew Morton]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
> Andries Brouwer <aebr@win.tue.nl> writes:
> 
> > 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.
> 
> 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);
> 

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?

Re: [PATCH] Fix NR_KEYS off-by-one error

[Vojtech Pavlik]

On Mon, Jul 26, 2004 at 03:43:27PM -0700, Andrew Morton wrote:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
> >
> > Andries Brouwer <aebr@win.tue.nl> writes:
> > 
> > > 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.
> > 
> > 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);
> > 
> 
> 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.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Vojtech Pavlik <vojtech@suse.cz> writes:

> > 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).
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Fix NR_KEYS off-by-one error

[Vojtech Pavlik]

On Wed, Jul 28, 2004 at 01:37:00AM +0900, OGAWA Hirofumi wrote:

> Vojtech Pavlik <vojtech@suse.cz> writes:
> 
> > > 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).
 
We can do both, then, if that helps.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Vojtech Pavlik <vojtech@suse.cz> writes:

> On Wed, Jul 28, 2004 at 01:37:00AM +0900, OGAWA Hirofumi wrote:
> 
> > Vojtech Pavlik <vojtech@suse.cz> writes:
> > 
> > > > 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).
>  
> We can do both, then, if that helps.

I'm confused. Sorry.

What is meaning of both? The following patch?  If so, "if (i >= NR_KEYS)"
part is never true, because "i" is unsigned char.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

diff -puN include/linux/keyboard.h~nr_keys-off-by-one include/linux/keyboard.h
--- linux-2.6.8-rc2/include/linux/keyboard.h~nr_keys-off-by-one	2004-07-26 00:26:59.000000000 +0900
+++ linux-2.6.8-rc2-hirofumi/include/linux/keyboard.h	2004-07-26 00:26:59.000000000 +0900
@@ -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. */
diff -puN drivers/char/vt_ioctl.c~nr_keys-off-by-one drivers/char/vt_ioctl.c
--- linux-2.6.8-rc2/drivers/char/vt_ioctl.c~nr_keys-off-by-one	2004-07-28 03:18:43.000000000 +0900
+++ linux-2.6.8-rc2-hirofumi/drivers/char/vt_ioctl.c	2004-07-28 03:22:24.000000000 +0900
@@ -82,6 +82,8 @@ do_kdsk_ioctl(int cmd, struct kbentry __
 
 	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
 		return -EFAULT;
+	if (i >= NR_KEYS)
+		return -EINVAL;
 
 	switch (cmd) {
 	case KDGKBENT:
_

Re: [PATCH] Fix NR_KEYS off-by-one error

[Andries Brouwer]

On Sat, Jul 17, 2004 at 03:23:27PM +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.
> 
> 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]);

Hmm. Yes.

In 2.4 the preceding code is

        if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
                return -EINVAL;

It looks like somebody removed this test in 2.6. Bad.
No doubt an error caused by "fixing" gcc warnings.
Shame on akpm.

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.

Andries

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Andries Brouwer <aebr@win.tue.nl> writes:

> 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.

OK. The patch is the following.

Any comments or suggestions?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] 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 <hirofumi@mail.parknet.co.jp>
---

 drivers/char/vt_ioctl.c  |    6 ++++++
 include/linux/keyboard.h |    2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff -puN include/linux/keyboard.h~nr_keys-off-by-one include/linux/keyboard.h
--- linux-2.6.8-rc2/include/linux/keyboard.h~nr_keys-off-by-one	2004-07-28 03:37:12.000000000 +0900
+++ linux-2.6.8-rc2-hirofumi/include/linux/keyboard.h	2004-07-28 03:37:12.000000000 +0900
@@ -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. */
diff -puN drivers/char/vt_ioctl.c~nr_keys-off-by-one drivers/char/vt_ioctl.c
--- linux-2.6.8-rc2/drivers/char/vt_ioctl.c~nr_keys-off-by-one	2004-07-29 01:31:12.000000000 +0900
+++ linux-2.6.8-rc2-hirofumi/drivers/char/vt_ioctl.c	2004-07-29 01:35:23.000000000 +0900
@@ -83,6 +83,12 @@ do_kdsk_ioctl(int cmd, struct kbentry __
 	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
 		return -EFAULT;
 
+#if NR_KEYS != 256 || MAX_NR_KEYMAPS != 256
+#error "you should check this too"
+	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
+		return -EINVAL;
+#endif
+
 	switch (cmd) {
 	case KDGKBENT:
 		key_map = key_maps[s];
_

Re: [PATCH] Fix NR_KEYS off-by-one error

[Paul Jackson]

OGAWA Hirofumi writes:
> Any comments or suggestions?

How about coding more of this in C instead of in C preprocessor:

--- 2.6.7-mm7/drivers/char/vt_ioctl.c	2004-07-10 17:21:07.000000000 -0700
+++ 2.6.7-mm7.new/drivers/char/vt_ioctl.c	2004-07-28 13:32:47.000000000 -0700
@@ -71,18 +71,26 @@ unsigned char keyboard_type = KB_101;
 #define GPLAST 0x3df
 #define GPNUM (GPLAST - GPFIRST + 1)
 
-#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;
 	ushort *key_map, val, ov;
+	unsigned char s, i;
+	unsigned short v;
 
 	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
 		return -EFAULT;
 
+	s = tmp.kb_table;
+	i = tmp.kb_index;
+	v = tmp.kb_value;
+
+	if (s >= ARRAY_SIZE(key_maps))
+		return -EINVAL;
+	if (i >= ARRAY_SIZE(key_map))
+		return -EINVAL;
+
 	switch (cmd) {
 	case KDGKBENT:
 		key_map = key_maps[s];
@@ -155,9 +163,6 @@ do_kdsk_ioctl(int cmd, struct kbentry __
 	}
 	return 0;
 }
-#undef i
-#undef s
-#undef v
 
 static inline int 
 do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc, int perm)


-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Paul Jackson <pj@sgi.com> writes:

> -#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;
>  	ushort *key_map, val, ov;
> +	unsigned char s, i;
> +	unsigned short v;
>  
>  	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
>  		return -EFAULT;
>  
> +	s = tmp.kb_table;
> +	i = tmp.kb_index;
> +	v = tmp.kb_value;

Yes, s/i/v macro is ugly.  But I think it shouldn't, because it limits
the range when struct kbentry was changed.  (I think this change is likely)

Perhaps only expand the macro... hmm..

> +	if (s >= ARRAY_SIZE(key_maps))
> +		return -EINVAL;
> +	if (i >= ARRAY_SIZE(key_map))
> +		return -EINVAL;

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.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Fix NR_KEYS off-by-one error

[Paul Jackson]

Thank-you for considering my comments.

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

I'm not sure I understand this comment -- but I am guessing that
you mean that:

> +	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;

However, if I have guessed incorrectly, then this idea may make no
sense.  If so, sorry about that.


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

Yes - good point.


> 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?


> 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 won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Paul Jackson <pj@sgi.com> writes:

> > +	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;

Yes.

> > 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?

Probably gcc not warn, I think.

> > 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.

Yes, I agree. But sorry, honestly I didn't see any big cleanup in your patch.
It's still using s/v/i... it is same readability at least for me...
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Fix NR_KEYS off-by-one error

[Paul Jackson]

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

That could well be <grin>.  Beauty is in the eye of the beholder.

I figured that replacing the #define's with local variables
was worth a couple of style points, and that explicitly
checking that the user provided values were within limits,
all the time, no ifdefs, was more straight forward.

But it is difficult to establish such preferences with certainty.

Again, thanks for considering my reply.

Do as you think best.  I am afraid I have no more wisdom (if
ever I had any ...).

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] Fix NR_KEYS off-by-one error

[Andrew Morton]

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

Re: [PATCH] Fix NR_KEYS off-by-one error

[Paul Jackson]

Andrew:
> ... 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.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Paul Jackson <pj@sgi.com> writes:

> 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.

Could you please post your lastest patch? It seems that my patch does
not satisfy peoples.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] Fix NR_KEYS off-by-one error

[Paul Jackson]

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

OGAWA Hirofumi writes:
> Could you please post your lastest patch? It seems that my patch does
> not satisfy peoples.

Hmmm ...

As is often the case, Andrew knows more than he lets on.


OGAWA,

I cannot post a latest patch.  I do not know enough to do so.

I attempted some code readability comments, but probably I should not
have, since I don't have the knowledge of this code that I should have
in order to test it, nor to know by reading it what works.

You replied, a couple of messages ago, with entirely sensible comments
to my posting.  Since then, I have been doing my best to remove myself
from this thread.

Unless you have something else you wish to propose, please tell Andrew
to go with your patch, which (just to avoid confusion) I copy again
below.

Your patch satisfies me just fine now.

Please let Andrew know if he should take it.

My apologies for making a confusion of this.

Thank-you for your good work and your patience.


[PATCH] 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 <hirofumi@mail.parknet.co.jp>
---

 drivers/char/vt_ioctl.c  |    6 ++++++
 include/linux/keyboard.h |    2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff -puN include/linux/keyboard.h~nr_keys-off-by-one include/linux/keyboard.h
--- linux-2.6.8-rc2/include/linux/keyboard.h~nr_keys-off-by-one	2004-07-28 03:37:12.000000000 +0900
+++ linux-2.6.8-rc2-hirofumi/include/linux/keyboard.h	2004-07-28 03:37:12.000000000 +0900
@@ -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. */
diff -puN drivers/char/vt_ioctl.c~nr_keys-off-by-one drivers/char/vt_ioctl.c
--- linux-2.6.8-rc2/drivers/char/vt_ioctl.c~nr_keys-off-by-one	2004-07-29 01:31:12.000000000 +0900
+++ linux-2.6.8-rc2-hirofumi/drivers/char/vt_ioctl.c	2004-07-29 01:35:23.000000000 +0900
@@ -83,6 +83,12 @@ do_kdsk_ioctl(int cmd, struct kbentry __
 	if (copy_from_user(&tmp, user_kbe, sizeof(struct kbentry)))
 		return -EFAULT;
 
+#if NR_KEYS != 256 || MAX_NR_KEYMAPS != 256
+#error "you should check this too"
+	if (i >= NR_KEYS || s >= MAX_NR_KEYMAPS)
+		return -EINVAL;
+#endif
+
 	switch (cmd) {
 	case KDGKBENT:
 		key_map = key_maps[s];
_
-

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] Fix NR_KEYS off-by-one error

[Vojtech Pavlik]

Hi!

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, and thus we ended up with the maximum value that
kept all software happy.

Now somewhere in the process (I assume in the 512 stage, but could be
the 256 stage as well), the kernel produced warnings about impossible
comparisons.

Thus the comparison(s) were dropped, by Andrew.

Then we finalized on 255, and the comparison is needed again to prevent
overflowing the keymap arrays.

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.

Any comments?

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] Fix NR_KEYS off-by-one error

[Andries Brouwer]

On Fri, Jul 30, 2004 at 10:07:57AM +0200, Vojtech Pavlik wrote:

> 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?

> ...
> 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?".

Andries

Re: [PATCH] Fix NR_KEYS off-by-one error

[Vojtech Pavlik]

On Fri, Jul 30, 2004 at 10:41:03AM +0200, Andries Brouwer wrote:

> > 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?

Unfortunately I don't remember. I'll dig my mailbox to see if I can find
anything.

> > 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?".


-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] Fix NR_KEYS off-by-one error

[Vojtech Pavlik]

On Fri, Jul 30, 2004 at 10:41:03AM +0200, Andries Brouwer wrote:

> On Fri, Jul 30, 2004 at 10:07:57AM +0200, Vojtech Pavlik wrote:
> 
> > 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?

Hmm, so bk says it was the other way around:

128, 256, 512, 255

And 256 probably worked for most people, except loadkeys had to be
changed not to #define NR_KEYS itself. So now I believe that 256 could
actually be safe.

> 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?".

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR