[Qemu-devel] [PATCH] keymaps: detect recursive keyboard layout file

[Qemu-devel] [PATCH] keymaps: detect recursive keyboard layout file

[Li Qiang]

When the parse_keyboard_layout() find a "include " line
in the keyboard layout file, it will call parse_keyboard_layout()
to perform a recursive parse. If the keyboard layout is malformed
by adding a line include itself, this can cause an infinite parse.
Thus cause qemu a segv. This patch avoid this.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 ui/keymaps.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/ui/keymaps.c b/ui/keymaps.c
index 085889b555..564893a9f3 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -38,6 +38,8 @@ struct kbd_layout_t {
     GHashTable *hash;
 };
 
+GList *keyboard_files;
+
 static int get_keysym(const name2keysym_t *table,
                       const char *name)
 {
@@ -80,6 +82,11 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
     trace_keymap_add(keysym, keycode, line);
 }
 
+static gint compare_string(gconstpointer a, gconstpointer b)
+{
+    return g_strcmp0(a, b);
+}
+
 static int parse_keyboard_layout(kbd_layout_t *k,
                                  const name2keysym_t *table,
                                  const char *language, Error **errp)
@@ -94,12 +101,18 @@ static int parse_keyboard_layout(kbd_layout_t *k,
     filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language);
     trace_keymap_parse(filename);
     f = filename ? fopen(filename, "r") : NULL;
-    g_free(filename);
     if (!f) {
+        g_free(filename);
         error_setg(errp, "could not read keymap file: '%s'", language);
         return -1;
     }
 
+    if (g_list_find_custom(keyboard_files, filename, compare_string)) {
+        error_setg(errp, "find recursive keyboard layout: %s'", filename);
+        g_free(filename);
+        return -1;
+    }
+    keyboard_files = g_list_append(keyboard_files, filename);
     for(;;) {
         if (fgets(line, 1024, f) == NULL) {
             break;
@@ -168,6 +181,8 @@ static int parse_keyboard_layout(kbd_layout_t *k,
     ret = 0;
 out:
     fclose(f);
+    keyboard_files = g_list_remove(keyboard_files, filename);
+    g_free(filename);
     return ret;
 }
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH] keymaps: detect recursive keyboard layout file

[Gerd Hoffmann]

On Thu, Nov 15, 2018 at 01:04:23AM -0800, Li Qiang wrote:
> When the parse_keyboard_layout() find a "include " line
> in the keyboard layout file, it will call parse_keyboard_layout()
> to perform a recursive parse. If the keyboard layout is malformed
> by adding a line include itself, this can cause an infinite parse.
> Thus cause qemu a segv. This patch avoid this.

Hmm.  Most keymap files are generated by qemu-keymap these days and do
not use includes in the first place.  Three are left over: nl-be, sl,
sv.

Looking at them it seems like nl-be is not functional, it just includes
"common" and doesn't define any mappings.  For sl and sv I have no clue
what keymap they represent.

So I'd suggest to just remove support for "include", drop the nl-be map,
fix the sl and sv maps that they don't need "include" any more.  Either
just replace the "include" statement with the content of the "common"
file.  Or, if someone has a clue what keyboard layout these keymaps are
for, add rules to the Makefile and update them using then using
qemu-keymap.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] keymaps: detect recursive keyboard layout file

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2018年11月15日周四 下午6:15写道：

> On Thu, Nov 15, 2018 at 01:04:23AM -0800, Li Qiang wrote:
> > When the parse_keyboard_layout() find a "include " line
> > in the keyboard layout file, it will call parse_keyboard_layout()
> > to perform a recursive parse. If the keyboard layout is malformed
> > by adding a line include itself, this can cause an infinite parse.
> > Thus cause qemu a segv. This patch avoid this.
>
> Hmm.  Most keymap files are generated by qemu-keymap these days and do
> not use includes in the first place.  Three are left over: nl-be, sl,
> sv.
>
> Looking at them it seems like nl-be is not functional, it just includes
> "common" and doesn't define any mappings.  For sl and sv I have no clue
> what keymap they represent.
>
> So I'd suggest to just remove support for "include", drop the nl-be map,
> fix the sl and sv maps that they don't need "include" any more.  Either
> just replace the "include" statement with the content of the "common"
> file.  Or, if someone has a clue what keyboard layout these keymaps are
> for, add rules to the Makefile and update them using then using
> qemu-keymap.
>


Hello Gerd,

Thanks for your advice.
Here my consideration is for the '-k' option can specific any file.
Though it seems too radical,  I consider every input can't be trusted.
Anyway, it's ok you ingore this patch.

Thanks,
Li Qiang




>
> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH] keymaps: detect recursive keyboard layout file

[Markus Armbruster]

Li Qiang <liq3ea@gmail.com> writes:

> When the parse_keyboard_layout() find a "include " line
> in the keyboard layout file, it will call parse_keyboard_layout()
> to perform a recursive parse. If the keyboard layout is malformed
> by adding a line include itself, this can cause an infinite parse.
> Thus cause qemu a segv. This patch avoid this.
>
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
>  ui/keymaps.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/ui/keymaps.c b/ui/keymaps.c
> index 085889b555..564893a9f3 100644
> --- a/ui/keymaps.c
> +++ b/ui/keymaps.c
> @@ -38,6 +38,8 @@ struct kbd_layout_t {
>      GHashTable *hash;
>  };
>  
> +GList *keyboard_files;
> +
>  static int get_keysym(const name2keysym_t *table,
>                        const char *name)
>  {
> @@ -80,6 +82,11 @@ static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
>      trace_keymap_add(keysym, keycode, line);
>  }
>  
> +static gint compare_string(gconstpointer a, gconstpointer b)
> +{
> +    return g_strcmp0(a, b);
> +}
> +
>  static int parse_keyboard_layout(kbd_layout_t *k,
>                                   const name2keysym_t *table,
>                                   const char *language, Error **errp)
> @@ -94,12 +101,18 @@ static int parse_keyboard_layout(kbd_layout_t *k,
>      filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language);
>      trace_keymap_parse(filename);
>      f = filename ? fopen(filename, "r") : NULL;
> -    g_free(filename);
>      if (!f) {
> +        g_free(filename);
>          error_setg(errp, "could not read keymap file: '%s'", language);
>          return -1;
>      }
>  
> +    if (g_list_find_custom(keyboard_files, filename, compare_string)) {
> +        error_setg(errp, "find recursive keyboard layout: %s'", filename);

Suggest something like "Inclusion loop for %s".

> +        g_free(filename);
> +        return -1;
> +    }
> +    keyboard_files = g_list_append(keyboard_files, filename);
>      for(;;) {
>          if (fgets(line, 1024, f) == NULL) {
>              break;
> @@ -168,6 +181,8 @@ static int parse_keyboard_layout(kbd_layout_t *k,
>      ret = 0;
>  out:
>      fclose(f);
> +    keyboard_files = g_list_remove(keyboard_files, filename);
> +    g_free(filename);
>      return ret;
>  }

There's no real need to make @keyboard_files global.  I'd make it local
to init_keyboard_layout(), and pass it as argument to
parse_keyboard_layout().  Matter of taste.  Gerd is the maintainer, not
me.

Re: [Qemu-devel] [PATCH] keymaps: detect recursive keyboard layout file

[Li Qiang]

Markus Armbruster <armbru@redhat.com> 于2018年11月15日周四 下午9:29写道：

> Li Qiang <liq3ea@gmail.com> writes:
>
> > When the parse_keyboard_layout() find a "include " line
> > in the keyboard layout file, it will call parse_keyboard_layout()
> > to perform a recursive parse. If the keyboard layout is malformed
> > by adding a line include itself, this can cause an infinite parse.
> > Thus cause qemu a segv. This patch avoid this.
> >
> > Signed-off-by: Li Qiang <liq3ea@gmail.com>
> > ---
> >  ui/keymaps.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui/keymaps.c b/ui/keymaps.c
> > index 085889b555..564893a9f3 100644
> > --- a/ui/keymaps.c
> > +++ b/ui/keymaps.c
> > @@ -38,6 +38,8 @@ struct kbd_layout_t {
> >      GHashTable *hash;
> >  };
> >
> > +GList *keyboard_files;
> > +
> >  static int get_keysym(const name2keysym_t *table,
> >                        const char *name)
> >  {
> > @@ -80,6 +82,11 @@ static void add_keysym(char *line, int keysym, int
> keycode, kbd_layout_t *k)
> >      trace_keymap_add(keysym, keycode, line);
> >  }
> >
> > +static gint compare_string(gconstpointer a, gconstpointer b)
> > +{
> > +    return g_strcmp0(a, b);
> > +}
> > +
> >  static int parse_keyboard_layout(kbd_layout_t *k,
> >                                   const name2keysym_t *table,
> >                                   const char *language, Error **errp)
> > @@ -94,12 +101,18 @@ static int parse_keyboard_layout(kbd_layout_t *k,
> >      filename = qemu_find_file(QEMU_FILE_TYPE_KEYMAP, language);
> >      trace_keymap_parse(filename);
> >      f = filename ? fopen(filename, "r") : NULL;
> > -    g_free(filename);
> >      if (!f) {
> > +        g_free(filename);
> >          error_setg(errp, "could not read keymap file: '%s'", language);
> >          return -1;
> >      }
> >
> > +    if (g_list_find_custom(keyboard_files, filename, compare_string)) {
> > +        error_setg(errp, "find recursive keyboard layout: %s'",
> filename);
>
> Suggest something like "Inclusion loop for %s".
>
> > +        g_free(filename);
> > +        return -1;
> > +    }
> > +    keyboard_files = g_list_append(keyboard_files, filename);
> >      for(;;) {
> >          if (fgets(line, 1024, f) == NULL) {
> >              break;
> > @@ -168,6 +181,8 @@ static int parse_keyboard_layout(kbd_layout_t *k,
> >      ret = 0;
> >  out:
> >      fclose(f);
> > +    keyboard_files = g_list_remove(keyboard_files, filename);
> > +    g_free(filename);
> >      return ret;
> >  }
>
> There's no real need to make @keyboard_files global.  I'd make it local
> to init_keyboard_layout(), and pass it as argument to
> parse_keyboard_layout().



Thanks Markus,
Finally, Gerd decide remove the support of "include" in keymaps.

Thanks,
Li Qiang


> Matter of taste.  Gerd is the maintainer, not
> me.
>