Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 17 Sep 2022 16:39:07 +0900
From:   Jiho Chu <jiho.chu@samsung.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     arnd@arndb.de, linux-kernel@vger.kernel.org,
        yelini.jeong@samsung.com, myungjoo.ham@samsung.com
Subject: Re: [PATCH 3/9] trinity: Add load/unload IDU files
Message-Id: <20220917163907.3dc80493dd21f3f2e2fbff4c@samsung.com>
In-Reply-To: <YuE6IrM/8slDjsdL@kroah.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20220725065308.2457024-1-jiho.chu@samsung.com>
        <CGME20220725065309epcas1p20c847655e7332c818fc0fd2c50fb0e27@epcas1p2.samsung.com>
        <20220725065308.2457024-4-jiho.chu@samsung.com> <YuE6IrM/8slDjsdL@kroah.com>
Precedence: bulk

On Wed, 27 Jul 2022 15:14:10 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Jul 25, 2022 at 03:53:02PM +0900, Jiho Chu wrote:
> > +static int triv2_idu_load_file(struct trinity_driver *drv, const char *dirpath,
> > +			       const char *file_name,
> > +			       struct trinity_resv_mem *sector)
> > +{
> > +	struct device *dev = drv_to_dev_ptr(drv);
> > +	struct trinity_resv_mem mem;
> > +	char filepath[NAME_MAX];
> > +	struct kstat *stat;
> > +	struct file *filp;
> > +	loff_t pos = 0;
> > +	size_t size;
> > +	int ret;
> > +
> > +	dev = drv_to_dev_ptr(drv);
> > +	stat = vmalloc(sizeof(*stat));
> > +	if (stat == NULL)
> > +		return -ENOMEM;
> > +
> > +	/* if dirpath is null, use the default path */
> > +	if (dirpath)
> > +		snprintf(filepath, NAME_MAX, "%s/%s", dirpath, file_name);
> > +	else
> > +		snprintf(filepath, NAME_MAX, TRIV2_IDU_DIRPATH_FMT "/%s",
> > +			 utsname()->release, file_name);
> > +
> > +	filp = filp_open(filepath, O_RDONLY, 0400);
> 
> That is cute.  And totally not ok.
> 
> Please never do this, that is not how to properly load a firmware blob
> in the kernel.  This is racy and broken and probably a huge security
> hole.
> 
> Heck, I wrote an article about this very topic, way back in 2005, with
> the title of, "Things you should never do in the kernel" and can be seen
> here:
> 	https://protect2.fireeye.com/v1/url?k=9f82c8ca-ff605597-9f834385-000babd9f1ba-3ee71f9f013fb8d9&q=1&e=0963e638-a9ed-43d0-95e3-adfcbdba2425&u=https%3A%2F%2Fwww.linuxjournal.com%2Farticle%2F8110
> 
> This should not be news to anyone, again, never do this.
> 
> thanks,
> 
> greg k-h
> 

Hi, greg
I just resent second revision of the driver.
As your reference, reading user space file mechnism is changed to use IOCTL call.
And many of your reviews (abstaction, open count, doc,, ) are very helpful and fixed in the revision.
If they are modified in wrong way, please let me know.

Thanks for your review.

Thanks,
Jiho Chu