From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pavel Herrmann <morpheus.ibis@gmail.com>
Date: Fri, 31 Aug 2012 16:25:24 +0200
Subject: [U-Boot] [PATCH] Loop block device for sandbox
In-Reply-To: <201208311457.41697.marex@denx.de>
References: <1346255203-1225-1-git-send-email-morpheus.ibis@gmail.com>
	<8644739.FHpg2PgWCH@merom> <201208311457.41697.marex@denx.de>
Message-ID: <1452774.McanHG5pJf@bloomfield>
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 Friday 31 of August 2012 14:57:41 Marek Vasut wrote:
> > > > > > > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt,
> > > > > > > > void *buffer)
> > > > > > > > +{
> > > > > > > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > > > > > > +	int fd = (long) pdev->priv;
> > > > > > > 
> > > > > > > If pdev is NULL, this will crash
> > > > > > 
> > > > > > well, it isn't, at least not from the command - thats why you
> > > > > > define the number of ports in advance, you get "dev" already
> > > > > > range-checked
> > > > > 
> > > > > Range check is fine, but will pdev be inited? It's a pointer from
> > > > > some array.
> > > > 
> > > > init_sata is called first, so pdev is inited (see cmd_sata.c)
> > > 
> > > Unless it fails. Then what ?
> > 
> > the only way init can "fail" is if it gets a wrong device number (which
> > should not happen), or if it cannot open the file, in which case it still
> > sets pdev as -1.
> 
> If pdev is -1, then this explodes for certain, on unaligned access and on
> wrong ptr deref.

again, no

there is no pointer in this, pdev->priv is actually an int stored as void*, 
pdev itself is pointer to a global array.
which one does explode in your case?

> > > > > > > > +	memcpy(pdev->product, filenames[dev], namelen);
> > > > > > > > +	pdev->product[20] = 0;
> > > > > > > > +
> > > > > > > > +	if (fd != -1) {
> > > > > > > 
> > > > > > > And if "fd" is -1 ?
> > > > > > 
> > > > > > then all defaults to an invalid device, because you failed to open
> > > > > > the file, for whatever the reason.
> > > > > 
> > > > > At least the printf below will choke, since pdev->lba is uninited
> > > > 
> > > > not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by
> > > > not doing anything we get an empty device
> > > 
> > > I see ... shall we also move all these memcpy() calls in to if (fd !=
> > > -1)
> > > then?
> > 
> > I'd like to know that the device is a loopback, and what filename, not
> > just
> > that it failed to init
> 
> But are such data used somewhere further down the road?

yes, "sata info" for example

Pavel Herrmann