linux-kernel - Re: [patch]fastboot: remove duplicate unpack_to

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]

Date:	Wed, 13 Aug 2008 09:45:03 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Shaohua Li <shaohua.li@...el.com>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Arjan van de Ven <arjan@...radead.org>
Subject: Re: [patch]fastboot: remove duplicate unpack_to_rootfs()


* Shaohua Li <shaohua.li@...el.com> wrote:

> we check if initrd is initramfs first and then do real unpack. The 
> check isn't required, we can directly do unpack. If initrd isn't 
> initramfs, we can remove garbage. In my laptop, this saves 0.1s boot 
> time. This penalizes non-initramfs case, but now initramfs is mostly 
> widely used.

clever concept!

a few observations about the cleanup function:

> +static void __init clean_rootfs(void)
> +{
> +	int fd = sys_open("/", O_RDONLY, 0);

can this ever fail?

> +	void *buf = malloc(1024);

no error checking for buf==NULL.

> +	struct linux_dirent64 *dirp = buf;
> +	int count;
> +
> +	memset(buf, 0, PAGE_SIZE);

overflow: clearly allocating a 1024 bytes buffer and then clearing 4096 
bytes isnt that good?

you could introduce a default-off CONFIG_DEBUG_ROOTFS_CLEANUP option 
that does two runs of unpack_to_rootfs() and inserts an artificial 
clean_rootfs() inbetween? Even if that debug patch doesnt get integrated 
its a good test for the cleanup function.

> +	count = sys_getdents64(fd, dirp, PAGE_SIZE);

... and then doing an up to 4096 bytes getdents into the buffer. A large 
enough initramfs will overflow this.

> +	while (count > 0) {
> +		while (count > 0) {
> +			struct stat st;
> +
> +			sys_newlstat(dirp->d_name, &st);

can this ever fail? If yes we should at least WARN_ON_ONCE().

> +			if (S_ISDIR(st.st_mode))
> +				sys_rmdir(dirp->d_name);
> +			else
> +				sys_unlink(dirp->d_name);
> +
> +			count -= dirp->d_reclen;

can this ever zero-underflow, with a sufficiently corrupted initramfs? 
We should check for 0 underflow to be sure.

> +			dirp = (void *)dirp + dirp->d_reclen;

likewise, we should size-overflow check this pointer. Failure modes of 
overrunning the buffer are subtle and hard to notice/track down.

> +		}
> +		dirp = buf;
> +		memset(buf, 0, 1024);
> +		count = sys_getdents64(fd, dirp, PAGE_SIZE);

overflow: we do a 4096 bytes getdents into a 1K buffer.

> +	}
> +
> +	sys_close(fd);
> +	free(buf);
> +}
> +
>  static int __init populate_rootfs(void)
>  {
>  	char *err = unpack_to_rootfs(__initramfs_start,
> @@ -531,13 +563,15 @@ static int __init populate_rootfs(void)
>  		int fd;
>  		printk(KERN_INFO "checking if image is initramfs...");
>  		err = unpack_to_rootfs((char *)initrd_start,
> -			initrd_end - initrd_start, 1);
> +			initrd_end - initrd_start, 0);
>  		if (!err) {
>  			printk(" it is\n");
> -			unpack_to_rootfs((char *)initrd_start,
> -				initrd_end - initrd_start, 0);
>  			free_initrd();
>  			return 0;
> +		} else {
> +			clean_rootfs();
> +			unpack_to_rootfs(__initramfs_start,
> +				 __initramfs_end - __initramfs_start, 0);
>  		}

the dry_run variable is now unused in unpack_to_rootfs() and could be 
eliminated.

>  		printk("it isn't (%s); looks like an initrd\n", err);
>  		fd = sys_open("/initrd.image", O_WRONLY|O_CREAT, 0700);

	Ingo
--
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/