Re: [PATCH 1/2] ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130

[ZhaoLong Wang]

Yes, that could happen. I was able to reproduce the problem despite the
low probability of triggering it.

This race between wear_leveling_work() and ubi_resize_volume() can cause
data corruption in the UBIFS running on the UBI volume.. ubi->volumes_lock
must be added to protect the update of eba_tbl in the ubi_eba_copy_leb().

I'll do a V2 patch later to fix this issue.

With appreciation
ZhaoLong Wang

> HI,
> > From: Guo Xuenan <guoxuenan@xxxxxxxxxx>
> >
> > When using ioctl interface to resize ubi volume, ubi_resize_volume will
> > resize eba table first, but not change vol->reserved_pebs in the same
> > atomic context which may cause concurrency access eba table.
> >
> > For example, When user do shrink ubi volume A calling ubi_resize_volume,
> > while the other thread is writing (volume B) and triggering 
> > wear-leveling,
> > which may calling ubi_write_fastmap, under these circumstances, KASAN 
> > may
> > report: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130.
> [...]
> > diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
> > index 2c867d16f89f..97294def01eb 100644
> > --- a/drivers/mtd/ubi/vmt.c
> > +++ b/drivers/mtd/ubi/vmt.c
> > @@ -408,6 +408,7 @@ int ubi_resize_volume(struct ubi_volume_desc 
> > *desc, int reserved_pebs)
> >       struct ubi_device *ubi = vol->ubi;
> >       struct ubi_vtbl_record vtbl_rec;
> >       struct ubi_eba_table *new_eba_tbl = NULL;
> > +    struct ubi_eba_table *old_eba_tbl = NULL;
> >       int vol_id = vol->vol_id;
> >         if (ubi->ro_mode)
> > @@ -453,10 +454,13 @@ int ubi_resize_volume(struct ubi_volume_desc 
> > *desc, int reserved_pebs)
> >               err = -ENOSPC;
> >               goto out_free;
> >           }
> > +
> >           ubi->avail_pebs -= pebs;
> >           ubi->rsvd_pebs += pebs;
> >           ubi_eba_copy_table(vol, new_eba_tbl, vol->reserved_pebs);
> > -        ubi_eba_replace_table(vol, new_eba_tbl);
> > +        old_eba_tbl = vol->eba_tbl;
> > +        vol->eba_tbl = new_eba_tbl;
> > +        vol->reserved_pebs = reserved_pebs;
> >           spin_unlock(&ubi->volumes_lock);
> >       }
> >   @@ -471,7 +475,9 @@ int ubi_resize_volume(struct ubi_volume_desc 
> > *desc, int reserved_pebs)
> >           ubi->avail_pebs -= pebs;
> >           ubi_update_reserved(ubi);
> >           ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
> > -        ubi_eba_replace_table(vol, new_eba_tbl);
> > +        old_eba_tbl = vol->eba_tbl;
> > +        vol->eba_tbl = new_eba_tbl;
> > +        vol->reserved_pebs = reserved_pebs;
> >           spin_unlock(&ubi->volumes_lock);
> >       }
> >   @@ -493,7 +499,6 @@ int ubi_resize_volume(struct ubi_volume_desc 
> > *desc, int reserved_pebs)
> >       if (err)
> >           goto out_acc;
> >   -    vol->reserved_pebs = reserved_pebs;
> >       if (vol->vol_type == UBI_DYNAMIC_VOLUME) {
> >           vol->used_ebs = reserved_pebs;
> >           vol->last_eb_bytes = vol->usable_leb_size;
> > @@ -501,19 +506,24 @@ int ubi_resize_volume(struct ubi_volume_desc 
> > *desc, int reserved_pebs)
> >               (long long)vol->used_ebs * vol->usable_leb_size;
> >       }
> >   +    /* destroy old table */
> > +    ubi_eba_destroy_table(old_eba_tbl);
> >       ubi_volume_notify(ubi, vol, UBI_VOLUME_RESIZED);
> >       self_check_volumes(ubi);
> >       return err;
> >     out_acc:
> > +    spin_lock(&ubi->volumes_lock);
> > +    vol->reserved_pebs = reserved_pebs - pebs;
> >       if (pebs > 0) {
> > -        spin_lock(&ubi->volumes_lock);
> >           ubi->rsvd_pebs -= pebs;
> >           ubi->avail_pebs += pebs;
> > -        spin_unlock(&ubi->volumes_lock);
> > +        ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
> > +    } else {
> > +        ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
> >       }
> > -    return err;
> > -
> > +    vol->eba_tbl = old_eba_tbl;
> > +    spin_unlock(&ubi->volumes_lock);
> >   out_free:
> >       ubi_eba_destroy_table(new_eba_tbl);
> >       return err;
>
>
> Besides that, it's better to protect 'vol->eba_tbl->entries' 
> assignment like:
> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> index 403b79d6efd5..5ae0c1bc6f41 100644
> --- a/drivers/mtd/ubi/eba.c
> +++ b/drivers/mtd/ubi/eba.c
> @@ -1450,7 +1450,9 @@ int ubi_eba_copy_leb(struct ubi_device *ubi, int 
> from, int to,
>         }
>
>         ubi_assert(vol->eba_tbl->entries[lnum].pnum == from);
> +       spin_lock(&ubi->volumes_lock);
>         vol->eba_tbl->entries[lnum].pnum = to;
> +       spin_unlock(&ubi->volumes_lock);
>
>  out_unlock_buf:
>         mutex_unlock(&ubi->buf_mutex);
>
> Otherwise, a race between wear_leveling_work and shrinking volume 
> could happen:
>
>  ubi_resize_volume         wear_leveling_worker
>   ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
> vol->eba_tbl->entries[lnum].pnum = to; // update old eba_tbl
>   vol->eba_tbl = new_eba_tbl