linux-kernel - Re: [PATCH] UBI: Fix crash in try_recover

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20161014124928.5a27ed59@bbrezillon>
Date:   Fri, 14 Oct 2016 12:49:28 +0200
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Richard Weinberger <richard@....at>,
        Brian Norris <computersforpeace@...il.com>,
        Artem Bityutskiy <dedekind1@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] UBI: Fix crash in try_recover_peb()

On Thu, 13 Oct 2016 16:24:58 +0200
Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:

> Hi Geert,
> 
> On Thu, 13 Oct 2016 16:05:36 +0200
> Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> 
> >     drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’:
> >     drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function
> > 
> > The pointer vid_hdr is indeed not initialized, leading to a crash when
> > it is dereferenced.
> > 
> > Fix this by obtaining the pointer from the VID buffer, like is done
> > everywhere else.  
> 
> Indeed, I don't know how I missed that one :-/.

That's really weird. I tried to recompile the kernel on my machine
(before applying your patch), to see if the compiler was complaining,
and it's not :-(.

I also coded simple functions with goto statements, to check that the
detection was working correctly, and it is.

Maybe you have some ideas why my compiler is failing to detect the
problem (I'm currently using the ubuntu cross-toolchain:
'arm-linux-gnueabi-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.1) 5.4.0
20160609').

> 
> > 
> > Fixes: 3291b52f9ff0acc8 ("UBI: introduce the VID buffer concept")
> > Signed-off-by: Geert Uytterhoeven <geert@...ux-m68k.org>  
> 
> One minor comment below, otherwise
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> 
> > ---
> > Completely untested. And I know nothing about UBI ;-)
> > ---
> >  drivers/mtd/ubi/eba.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> > index 95c4048a371e87b6..388e46be6ad92805 100644
> > --- a/drivers/mtd/ubi/eba.c
> > +++ b/drivers/mtd/ubi/eba.c
> > @@ -741,6 +741,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum,
> >  		goto out_put;
> >  	}
> >  
> > +	vid_hdr = ubi_get_vid_hdr(vidb);  
> 
> Can you move this assignment at variable declaration time?
> 
> >  	ubi_assert(vid_hdr->vol_type == UBI_VID_DYNAMIC);
> >  
> >  	mutex_lock(&ubi->buf_mutex);  
>