linux-kernel - Re: please revert "Revert "media: staging: atomisp: Remove driver""

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:   Fri, 29 May 2020 17:01:09 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Christoph Hellwig <hch@....de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: please revert "Revert "media: staging: atomisp: Remove driver""

Em Fri, 29 May 2020 16:09:07 +0200
Christoph Hellwig <hch@....de> escreveu:

> Hi Mauro and Greg,
> 
> the commit mentioned in the subject (commit id ad85094b293e in
> linux-next) contains the grave offense of adding a new set_fs address
> space override in "new" code.  It also doesn't have an Ack from Greg
> despite showing up in drives/staging, which looks very suspicious.
> 
> Please don't just add crap like this back if it doesn't pass the
> most basic sanity tests.

Hi Christoph,

Thanks for the tip about set_fs().

The part of the driver which calls set_fs() is under the compat32
handler.

This code is commented-out at the commit which reverted it:

	const struct v4l2_file_operations atomisp_fops = {
	        .owner = THIS_MODULE,
	        .open = atomisp_open,
	        .release = atomisp_release,
	        .mmap = atomisp_mmap,
	        .unlocked_ioctl = video_ioctl2,
	#ifdef CONFIG_COMPAT
	        /*
	         * There are problems with this code. Disable this for now.
	        .compat_ioctl32 = atomisp_compat_ioctl32,
	         */
	#endif
	      	.poll = atomisp_poll,
	};

So, there's not risk of calling it. Also, instead of calling
an atomisp-specific compat32, the driver should, instead, use
the standard V4L2 handler for it, once we can get rid of all
those new driver-specific ioctls. Most of which can probably be
replaced by the already existing ones.

But yeah, you're right, we should get rid of the places that
have set_fs() there. I'll add an additional patch for it to
get rid of the set_fs(), adding a FIXME there. 

I'll send a patch for it in a few.

Thanks,
Mauro