linux-kernel - Re: [syzbot] KASAN: use-after-free Read in em28xx_close

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Jul 2021 23:13:13 +0800
From:   Dongliang Mu <mudongliangabcd@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Hillf Danton <hdanton@...a.com>,
        syzbot <syzbot+005037419ebdf14e1d87@...kaller.appspotmail.com>,
        Igor Matheus Andrade Torrente <igormtorrente@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: [syzbot] KASAN: use-after-free Read in em28xx_close_extension

On Tue, Jul 27, 2021 at 10:15 PM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> On Tue, Jul 27, 2021 at 06:01:51PM +0800, Hillf Danton wrote:
> > Along the probe path,
> >
> > em28xx_usb_probe
> >   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >   retval = em28xx_init_dev(dev, udev, intf, nr);
> >     em28xx_init_extension(dev);
> >       em28xx_ir_init(struct em28xx *dev)
> >         kref_get(&dev->ref);
> >
> >   kref_init(&dev->ref);
>

Hi Dan,

I have developed a patch [1] to fix this crash. Would you like to help
me double-check if it correctly fixes the underlying bug?

[1] [PATCH v2] [media] em28xx-input: fix refcount bug in
em28xx_usb_disconnect  https://lkml.org/lkml/2021/7/19/263

> Good detective work.
>
> I've created a Smatch check to try find these.  It uses the fact that
> Smatch creates a bunch of fake assignments to set all the struct members
> of "dev" to zero.  Then it uses the modification hook to find the
> kref_init().  Those are sort of new uses for those hooks so that's quite
> fun.
>
> I'll test it out overnight and see how it works.
>
> drivers/media/usb/em28xx/em28xx-cards.c:4086 em28xx_usb_probe() warn: kref has already been modifed (see line 3979)
>
> regards,
> dan carpenter
>
> #include "smatch.h"
> #include "smatch_slist.h"
> #include "smatch_extra.h"
>
> static int my_id;
>
> STATE(fresh);
>
> static int get_line(struct sm_state *sm)
> {
>         struct sm_state *tmp;
>         int line = 0;
>
>         FOR_EACH_PTR(sm->possible, tmp) {
>                 if (tmp->state == &undefined &&
>                     tmp->line > line)
>                         line = tmp->line;
>         } END_FOR_EACH_PTR(tmp);
>
>         if (!line)
>                 return sm->line;
>         return line;
> }
>
> static void match_modify(struct sm_state *sm, struct expression *mod_expr)
> {
>         if (sm->state != &fresh &&
>             mod_expr &&
>             mod_expr->type == EXPR_CALL &&
>             sym_name_is("kref_init", mod_expr->fn))
>                 sm_warning("kref has already been modifed (see line %d)", get_line(sm));
>
>         set_state(my_id, sm->name, sm->sym, &undefined);
> }
>
> static bool is_alloc(struct expression *expr)
> {
>         static struct expression *ignore, *alloc_expr;
>         struct expression *right;
>
>         if (!expr || expr->type != EXPR_ASSIGNMENT || expr->op != '=')
>                 return false;
>         if (expr == ignore)
>                 return false;
>         if (expr == alloc_expr)
>                 return true;
>         right = strip_expr(expr->right);
>         if (right->type == EXPR_CALL &&
>             (sym_name_is("kzalloc", right->fn) ||
>              sym_name_is("kmalloc", right->fn))) {
>                 alloc_expr = expr;
>                 return true;
>         }
>         ignore = expr;
>         return false;
> }
>
> static void match_assign(struct expression *expr)
> {
>         char *name;
>
>         if (!is_alloc(get_faked_expression()))
>                 return;
>         name = expr_to_str(expr->left);
>         if (name && strstr(name, "refcount.refs.counter"))
>                 set_state_expr(my_id, expr->left, &fresh);
>         free_string(name);
> }
>
> void check_kref_init_too_late(int id)
> {
>         my_id = id;
>
>         add_hook(&match_assign, ASSIGNMENT_HOOK_AFTER);
>         add_modification_hook(my_id, &match_modify);
> }
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20210727141455.GM1931%40kadam.