Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 Jan 2021 08:57:03 -0700
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Andy Gross <agross@kernel.org>, Ohad Ben-Cohen <ohad@wizery.com>,
        Sibi Sankar <sibis@codeaurora.org>,
        linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] remoteproc: qcom_q6v5_mss: Validate p_filesz in ELF
 loader
Message-ID: <20210119155703.GA611676@xps15>
References: <20210107235053.745888-1-bjorn.andersson@linaro.org>
 <20210113212257.GB229796@xps15>
 <X/9uT0ld7OE5pPv4@builder.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <X/9uT0ld7OE5pPv4@builder.lan>
Precedence: bulk

On Wed, Jan 13, 2021 at 04:03:59PM -0600, Bjorn Andersson wrote:
> On Wed 13 Jan 15:22 CST 2021, Mathieu Poirier wrote:
> 
> > Hi Bjorn,
> > 
> > On Thu, Jan 07, 2021 at 03:50:53PM -0800, Bjorn Andersson wrote:
> > > Analog to the issue in the common mdt_loader code the MSS ELF loader
> > > does not validate that p_filesz bytes will fit in the memory region and
> > > that the loaded segments are not truncated. Fix this in the same way
> > > as proposed for the mdt_loader.
> > > 
> > > Fixes: 135b9e8d1cd8 ("remoteproc: qcom_q6v5_mss: Validate modem blob firmware size before load")
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >  drivers/remoteproc/qcom_q6v5_mss.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index 66106ba25ba3..2b59e0cbdce1 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -1210,6 +1210,14 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> > >  			goto release_firmware;
> > >  		}
> > >  
> > > +		if (phdr->p_filesz > phdr->p_memsz) {
> > > +			dev_err(qproc->dev,
> > > +				"refusing to load segment %d with p_filesz > p_memsz\n",
> > > +				i);
> > > +			ret = -EINVAL;
> > > +			break;
> > 
> > Based on the error handling for the above and below conditions, I would have
> > expected a "goto release_firmware" rather than a "break".
> > 
> 
> You're certainly right!
> 
> Yet another reason for the duplication between this function, the
> mdt_loader and the remoteproc_elf_loader is a bad idea - still not sure
> how to refactor any one of them to fit the three.

I feel the same way about duplication.  On the flip side I don't think we have
to take immediate action... The solution will come out at some point when there
is more convergence.  At least we know it is an area that needs attention.

> 
> Thank you,
> Bjorn
> 
> > > +		}
> > > +
> > >  		ptr = memremap(qproc->mpss_phys + offset, phdr->p_memsz, MEMREMAP_WC);
> > >  		if (!ptr) {
> > >  			dev_err(qproc->dev,
> > > @@ -1241,6 +1249,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
> > >  				goto release_firmware;
> > >  			}
> > >  
> > > +			if (seg_fw->size != phdr->p_filesz) {
> > > +				dev_err(qproc->dev,
> > > +					"failed to load segment %d from truncated file %s\n",
> > > +					i, fw_name);
> > > +				ret = -EINVAL;
> > > +				memunmap(ptr);
> > > +				break;
> > 
> > Same here.
> > 
> > > +			}
> > > +
> > >  			release_firmware(seg_fw);
> > >  		}
> > 
> > Thanks,
> > Mathieu
> > 
> > >  
> > > -- 
> > > 2.29.2
> > >