From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ladislav Michl <ladis@linux-mips.org>
Date: Wed, 22 Nov 2017 09:25:31 +0100
Subject: [U-Boot] [PATCH 1/1] ubifs: avoid possible NULL dereference
In-Reply-To: <20171122080936.32ECB12051D@gemini.denx.de>
References: <20171121184503.3193-1-xypron.glpk@gmx.de>
 <20171121211640.CE469120302@gemini.denx.de>
 <20171121212222.ryicwv6tyh5rye2e@lenoch>
 <20171122080936.32ECB12051D@gemini.denx.de>
Message-ID: <20171122082531.GA15601@lenoch>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Wed, Nov 22, 2017 at 09:09:36AM +0100, Wolfgang Denk wrote:
> Dear Ladislav,
> 
> In message <20171121212222.ryicwv6tyh5rye2e@lenoch> you wrote:
> > > > 
> > > > diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> > > > index 4465523d5f..313dee0579 100644
> > > > --- a/fs/ubifs/ubifs.c
> > > > +++ b/fs/ubifs/ubifs.c
> > > > @@ -403,8 +403,7 @@ static int ubifs_finddir(struct super_block *sb, char *dirname,
> > > >  	dir = kzalloc(sizeof(struct inode), 0);
> > > >  	if (!file || !dentry || !dir) {
> > > >  		printf("%s: Error, no memory for malloc!\n", __func__);
> > > > -		err = -ENOMEM;
> > > > -		goto out;
> > > > +		goto out_nomem;
> > > >  	}
> ...
> > > Should you not keep the "err = -ENOMEM;" setting?  Otherwise there
> > > is no indivcation that an error happened.
> > 
> > It is not obvious from the patch, but value of err is later discarded.
> > It serves sole purpose of printing debug notice.
> 
> So apparently we have a number of places in U-Boot where fatal
> errors (running out of memory) are just ignored and we continue as
> if nothing happened?

While I have to admit this code is not an example of clean coding,
it prints notice when trying to manipulate with file.

fs/ubifs/ubifs.c as whole needs to be revisited, above patch just
caused shit hitting the fan.

> THis is short-sighted at best. One day Pump Six will fail.
> 
> This is giving me the creepes. 

I was just pointing to the fact, that above mentioned patch does not
make it any worse. Btw, initial commit is even more amazing:

+       if (file)
+               free(file);
+       if (dentry)
+               free(dentry);
+       if (dir)
+               free(dir);
+
+       if (file->private_data)
+               kfree(file->private_data);
+       file->private_data = NULL;
+       file->f_pos = 2;
+       return 0;

.oO(I guess it is safe not pointing where above snippet is comming from,
but will review whole file as I'm going to use it for new board I have to
suport)

Best regards,
	ladis