linux-kernel - Re: [PATCH v11 08/21] dax,ext2: Replace ext2_clear_xip_target with dax_clear

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]

Message-ID: <20141016212234.GF11522@wil.cx>
Date:	Thu, 16 Oct 2014 17:22:34 -0400
From:	Matthew Wilcox <willy@...ux.intel.com>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 08/21] dax,ext2: Replace ext2_clear_xip_target with
 dax_clear_blocks

On Thu, Oct 16, 2014 at 12:05:25PM +0200, Mathieu Desnoyers wrote:
> > +int dax_clear_blocks(struct inode *inode, sector_t block, long size)
> > +{
> > +	struct block_device *bdev = inode->i_sb->s_bdev;
> > +	sector_t sector = block << (inode->i_blkbits - 9);
> 
> Is there a define e.g. SECTOR_SHIFT rather than using this hardcoded "9"
> value ?

Yeah ... in half a dozen drivers, so introducing them globally spews
warnings about redefining macros.  The '9' and '512' are sprinkled all
over the storage parts of the kernel, it's a complete flustercluck that
I wasn't about to try to unscrew.

> > +		while (count > 0) {
> > +			unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
> 
> unsigned -> unsigned int

Any particular reason?  Omitting it in some places helps stay within
the 80-column limit without sacrificing readability.

> > +		}
> > +	} while (size);
> 
> Just to stay on the safe side, can we do while (size > 0) ? Just in case
> an unforeseen issue makes size negative, and gets us in a very long loop.

If size < 0, we should BUG, because that means we've zeroed more than
we were asked to do, which is data corruption.

There's probably some other hardening we should do for this loop.
For example, if 'count' is < 512, it can go into an infinite loop.

        do {
                void *addr;
                unsigned long pfn;
                long count;

                count = bdev_direct_access(bdev, sector, &addr, &pfn, size);
                if (count < 0)
                        return count;
                while (count > 0) {
                        unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
                        if (pgsz > count)
                                pgsz = count;
                        if (pgsz < PAGE_SIZE)
                                memset(addr, 0, pgsz);
                        else
                                clear_page(addr);
                        addr += pgsz;
                        size -= pgsz;
                        count -= pgsz;
			BUG_ON(pgsz & 511);
                        sector += pgsz / 512;
                        cond_resched();
                }
		BUG_ON(size < 0);
        } while (size);

I think that should do the job ... ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/