On Tue, Mar 07, 2017 at 12:04:13PM +0100, Bjorn Andersson wrote:
> On Tue 07 Mar 03:01 CET 2017, Jonathan Neusch?fer wrote:
[...]
> If this is something that you will continue to hack on and you think
> anyone else will be interested in having then please do submit patches
> for it.
Ok, I'll give it a try.
> > > > [ 0.647743] qcom-smsm smsm: no smsm size info, using defaults
> > > > [ 0.647775] qcom-smsm smsm: unable to allocate shared state entry
> > > >
> > >
> > > Could you please confirm where in qcom_smem_alloc_global() we're
> > > failing? As far as I can tell we should fail with -EEXIST or if the
> > > passed "size" parameter is bogus -ENOMEM (but the default number of
> > > entries really should be less than the amount of free SMEM space).
> >
> > * qcom_smem_get returns -EPROBE_DEFER:
> >
> > void *ptr = ERR_PTR(-EPROBE_DEFER);
> > if (!__smem)
> > return ptr;
> >
> > * smsm_get_size_info prints "no smsm size info, using defaults\n"
> > * qcom_smem_alloc also returns -EPROBE_DEFER early.
> >
> >
> > BTW, I think smsm_get_size_info is using uninitialized memory here:
> >
> > size_t size; /* is uninitialized */
> > struct { ... } *info;
> >
> > /* qcom_smem_get returns early without setting size */
> > info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size);
> >
> > /*
> > * PTR_ERR(info) is not -ENOENT.
> > * size (still uninitialized) is compared with the size of the local
> > * struct defined above.
> > */
> > if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) {
> > ...
> > }
> >
>
> Thanks for your analysis!
>
> As you say the smsm driver is missing handling of probe deferral - which
> is wrong. Could you please propose a patch checking for EPROBE_DEFER and
> propagate this error as return value from probe()? (Without an error
> message)
I'm working on it.
Regarding the uninitialized memory read in "size != sizeof(*info)":
Is qcom_smem_get supposed to store a valid size value in any
non-probe-deferral case, or only in non-error cases?
Assuming the latter, I think "size != sizeof(*info)" should be changed
to "!IS_ERR(info) && size != sizeof(*info)", i.e. only check size if
qcom_smem_get returned success. Does that seem reasonable?
Jonathan Neuschäfer
Attachment:
signature.asc
Description: PGP signature
