linux-kernel - Re: Utility code to generate a VMUFAT filesystem

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: <201203250232.29843.vapier@gentoo.org>
Date:	Sun, 25 Mar 2012 02:32:27 -0400
From:	Mike Frysinger <vapier@...too.org>
To:	Adrian McMenamin <lkmladrian@...il.com>
Cc:	viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>,
	"Linux-sh" <linux-sh@...r.kernel.org>,
	Adrian McMenamin <adrianmcmenamin@...il.com>
Subject: Re: Utility code to generate a VMUFAT filesystem

On Wednesday 21 March 2012 00:41:17 Adrian McMenamin wrote:
> /*
>  * mkfs.vmufat.c - make a linux (VMUFAT) filesystem
>  *
>  * Copyright (c) 2012 Adrian McMenamin adrianmcmenamin@...il.com
>  * Licensed under Version 2 of the GNU General Public Licence
>  *
>  * Parts shamelessly copied from other mkfs code
>  * copyright Linus Torvalds and others
>  */

might be useful to sneak into tools/ or Documentation/ or fs/vmufat/

> void usage()

(void)

seems like most (if not just about all) funcs in here should be static

> int checkmount(char* device_name)
> {
> 	FILE * f;
> 	struct mntent * mnt;

kernel style wise, this really should be:
	char *foo;
	FILE *f;
	struct mntent *mnt;
	etc...

device_name here should be const

> 	if ((f = setmntent(_PATH_MOUNTED, "r")) == NULL)
> 		return;

there is libmount now, but that might be overkill for this small tool

> int readforbad(struct badblocklist** root, char* filename, int verbose)

filename should be const

> {
> 	int error = 0;
> 	FILE *listfile;
> 	int badblocks = 0;
> 	unsigned long blockno;
> 	...
> 
> 	while (!feof(listfile)) {
> 		if (fscanf(listfile, "%ld\n", &blockno) != 1) {

%ld is "signed long" but blockno is "unsigned long".  probably want %lu here 
and elsewhere in this func.

> void _fill_root_block(char* buf, const struct vmuparam* param)

incoming buf is "char *" ...

> 	uint16_t* wordbuf;
> 
> 	wordbuf = (uint16_t *)buf;

... which you cast up to "uint16_t *" ...

> int mark_root_block(int device_numb, const struct vmuparam *param, int
> verbose) {
> 	char zilches[BLOCKSIZE];
> 	int i, error = 0;
> 
> 	for (i = 0; i < BLOCKSIZE; i++)
> 		zilches[i] = '\0';
> 
> 	_fill_root_block(zilches, param);

... and is declared on the stack as "char *".  there are no alignment 
requirements here, and iirc, superh doesn't support unaligned loads.  so you 
should add gcc aligned attributes, or declare the buffer better:
	uint16_t zilches[BLOCKSIZE / 2];

> int zero_blocks(int device_numb, const struct vmuparam *param, int verbose)
> {
> 	char zilches[BLOCKSIZE];
> 	int i, error = -1;
> 
> 	for (i = 0; i < BLOCKSIZE; i++)
> 		zilches[i] = '\0';

memset(zilches, '\0', BLOCKSIZE);

> int scanforbad(int device_numb, struct badblocklist** root, int verbose)
> {
> 	int error = 0, i;
> 	struct badblocklist *lastbadblock = NULL;
> 	off_t size;
> 	long got;
> 	char buffer[BLOCKSIZE];
> 
> 	size = lseek(device_numb, 0, SEEK_END);

if you use fstat() here ...

> 	for (i = 0; i < size/BLOCKSIZE; i++)
> 	{
> 		if (verbose > 0)
> 			printf("Testing block %i\n", i);
> 		if (lseek(device_numb, i * BLOCKSIZE, SEEK_SET) !=
> 			i * BLOCKSIZE) {
> 			printf("Seek failed on device\n");
> 			error = -1;
> 			goto out;
> 		}
> 		got = read(device_numb, buffer, BLOCKSIZE);

... and pread() here, there's no need for the lseek()'s at all.

> 	if (lseek(device_numb, fatblock * BLOCKSIZE, SEEK_SET) < 0)
> 		goto out;
> 	if (read(device_numb, buffer, BLOCKSIZE) != BLOCKSIZE)
> 		goto out;

use pread() instead of lseek() && read()

> 	if (lseek(device_numb, fatblock * BLOCKSIZE, SEEK_SET) < 0)
> 		goto out;
> 	if (write(device_numb, buffer, BLOCKSIZE) != BLOCKSIZE)
> 		goto out;

and pwrite() instead of lseek() && write()

> 	if (checkmount(device_name) < 0)
> 		goto out;
> 
> 	if (stat(device_name, &statbuf) < 0) {
> 		printf("Cannot get status of %s\n", device_name);
> 		goto out;
> 	}
> 	if (!S_ISBLK(statbuf.st_mode)) {
> 		printf("%s must be a block device\n", device_name);
> 		goto out;
> 	}
> 	device_numb = open(device_name, O_RDWR|O_EXCL);

technically you've got a race condition here.  really should do the open() and 
then do fstat() on the fd you get back.

also, what's with the O_EXCL ?  that only makes sense with O_CREAT.

should you open+fstat before checking for the mount point ?  guess it doesn't 
really matter.

it's useful to be able to format regular files as filesystems especially for 
debugging.  most mkfs tools issue a warning or prompt by default, and using 
the -f (force) flag allows you to format regular files.
-mike

Download attachment "signature.asc " of type "application/pgp-signature" (837 bytes)