Re: [PATCH] soc: qcom: mdt_loader: Drop PT_LOAD check on hash segment

[Shawn Guo <shawn.guo@linaro.org>]

Hi Marijn,

Thanks much for the information!

On Tue, Aug 24, 2021 at 05:18:05PM +0200, Marijn Suijten wrote:
> Hi Shawn,
> 
> On 8/24/21 11:41 AM, Shawn Guo wrote:
> > From: Shawn Guo <shawn.guo@linaro.org>
> > 
> > It's been observed on Sony Xperia M4 Aqua phone, that wcnss firmware has
> > the type of the second segment holding hashes just be PT_LOAD.  So drop
> > the check on phdrs[1].p_type to get it go on that phone.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >   drivers/soc/qcom/mdt_loader.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> > index eba7f76f9d61..6034cd8992b0 100644
> > --- a/drivers/soc/qcom/mdt_loader.c
> > +++ b/drivers/soc/qcom/mdt_loader.c
> > @@ -98,7 +98,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
> >   	if (ehdr->e_phnum < 2)
> >   		return ERR_PTR(-EINVAL);
> > -	if (phdrs[0].p_type == PT_LOAD || phdrs[1].p_type == PT_LOAD)
> > +	if (phdrs[0].p_type == PT_LOAD)
> >   		return ERR_PTR(-EINVAL);
> >   	if ((phdrs[1].p_flags & QCOM_MDT_TYPE_MASK) != QCOM_MDT_TYPE_HASH)
> > 
> 
> 
> Konrad (on the CC-list) originally came up with a similar patch to make his
> Sony phone boot (Xperia XZ, MSM8996).  We however concluded that this
> solution is wrong, for the simple fact that the code below expects a PT_NULL
> header with data in the right place.  Skipping this check most likely means
> that the code below will read out of bounds since the mdt file isn't large
> enough; this data is supposed to be read from an external file as indicated
> by the meaning of PT_LOAD.  We built a solution for this, and fortunately
> CAF independently submitted a similar solution to the lists a while ago
> which is more thorough:
> 
> https://lore.kernel.org/linux-arm-msm/1609968211-7579-1-git-send-email-sidgup@codeaurora.org/

While a thorough solution is good, I do not think my patch makes
anything worse or breaks anything, while fixing my issue on Sony Xperia
M4 Aqua.  All the current assumptions hold true for my WCNSS firmware,
only except that phdrs[1] gets a PT_LOAD type.

There is a blog[1] illustrating the situation quite nicely.  Again, the
only thing my WCNSS firmware differs from the illustration is that
hash segment gets a PT_LOAD type.

Skipping the check will not cause problem for firmwares we have seen,
because hash segment is duplicated in .mdt file, and we are using that
duplication instead of reading from .b01 file.

Shawn

[1] http://bits-please.blogspot.com/2016/04/exploring-qualcomms-secure-execution.html