| Message ID | 20240530102032.27179-3-quic_ekangupt@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add missing features to FastRPC driver | expand |
On Thu, May 30, 2024 at 03:50:20PM +0530, Ekansh Gupta wrote: > Incorrect remote arguments are getting passed when requesting for > capabilities from DSP. Describe why and how they are incorrect. > Also there is no requirement to update the > PD type as it might cause problems for any PD other than user PD. Also... means that these are two separate issues. There should be two separate commits. > In addition to this, the collected capability information is not > getting copied properly to user. Add changes to address these > problems and get correct DSP capabilities. > > Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities") > Cc: stable <stable@kernel.org> > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > drivers/misc/fastrpc.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 4028cb96bcf2..61389795f498 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr > args[0].length = sizeof(dsp_attr_buf_len); > args[0].fd = -1; > args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1]; > - args[1].length = dsp_attr_buf_len; > + args[1].length = dsp_attr_buf_len * sizeof(uint32_t); As you are skipping first entry, should there be (dsp_attr_buf_len - 1) * sizeof(uint32_t). > args[1].fd = -1; > - fl->pd = USER_PD; > > return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE, > FASTRPC_SCALARS(0, 1, 1), args); > @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, > if (!dsp_attributes) > return -ENOMEM; > > - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN); > + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES); So it looks like the argument was correct. It was passing length, not the number of attributes. The only thing to fix is that args[1].length should be dsp_attr_buf_len - sizeof(*dsp_attr_buf). > if (err == DSP_UNSUPPORTED_API) { > dev_info(&cctx->rpdev->dev, > "Warning: DSP capabilities not supported on domain: %d\n", domain); > @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) > if (err) > return err; > > - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability))) > + if (copy_to_user(argp, &cap, sizeof(cap))) > return -EFAULT; > > return 0; > -- > 2.43.0 >
On 30/05/2024 11:20, Ekansh Gupta wrote: > Incorrect remote arguments are getting passed when requesting for > capabilities from DSP. Also there is no requirement to update the > PD type as it might cause problems for any PD other than user PD. > In addition to this, the collected capability information is not > getting copied properly to user. Add changes to address these > problems and get correct DSP capabilities. > > Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities") > Cc: stable <stable@kernel.org> > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > drivers/misc/fastrpc.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 4028cb96bcf2..61389795f498 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr > args[0].length = sizeof(dsp_attr_buf_len); > args[0].fd = -1; > args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1]; > - args[1].length = dsp_attr_buf_len; > + args[1].length = dsp_attr_buf_len * sizeof(uint32_t); This does not look correct, we have allocated buffer of size FASTRPC_MAX_DSP_ATTRIBUTES_LEN which is already (sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES) now this patch multiplies with again sizeof(uint32_t), this is going to send dsp incorrect size for buffer and overrun the buffer size. > args[1].fd = -1; > - fl->pd = USER_PD; another patch may be. > > return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE, > FASTRPC_SCALARS(0, 1, 1), args); > @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, > if (!dsp_attributes) > return -ENOMEM; > > - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN); > + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES); You change this again to send FASTRPC_MAX_DSP_ATTRIBUTES instead of FASTRPC_MAX_DSP_ATTRIBUTES_LEN but why? > if (err == DSP_UNSUPPORTED_API) { > dev_info(&cctx->rpdev->dev, > "Warning: DSP capabilities not supported on domain: %d\n", domain); > @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) > if (err) > return err; > > - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability))) > + if (copy_to_user(argp, &cap, sizeof(cap))) Why are we copying the full struct here? All that user needs is cap.capability? --srini > return -EFAULT; > > return 0;
On 5/30/2024 4:29 PM, Dmitry Baryshkov wrote: > On Thu, May 30, 2024 at 03:50:20PM +0530, Ekansh Gupta wrote: >> Incorrect remote arguments are getting passed when requesting for >> capabilities from DSP. > Describe why and how they are incorrect. Sure, I'll update this information in the next spin. > >> Also there is no requirement to update the >> PD type as it might cause problems for any PD other than user PD. > Also... means that these are two separate issues. There should be two > separate commits. Okay, I'll separate out the PD type change. > >> In addition to this, the collected capability information is not >> getting copied properly to user. Add changes to address these >> problems and get correct DSP capabilities. >> >> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities") >> Cc: stable <stable@kernel.org> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >> --- >> drivers/misc/fastrpc.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index 4028cb96bcf2..61389795f498 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr >> args[0].length = sizeof(dsp_attr_buf_len); >> args[0].fd = -1; >> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1]; >> - args[1].length = dsp_attr_buf_len; >> + args[1].length = dsp_attr_buf_len * sizeof(uint32_t); > As you are skipping first entry, should there be (dsp_attr_buf_len - 1) > * sizeof(uint32_t). This was done in the next patch of the series, I'll bring it here. > >> args[1].fd = -1; >> - fl->pd = USER_PD; >> >> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE, >> FASTRPC_SCALARS(0, 1, 1), args); >> @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, >> if (!dsp_attributes) >> return -ENOMEM; >> >> - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN); >> + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES); > So it looks like the argument was correct. It was passing length, not > the number of attributes. The only thing to fix is that args[1].length > should be dsp_attr_buf_len - sizeof(*dsp_attr_buf). args[0] is expected to carry the information about the total number of attributes to be copied from DSP and not the information about the size to be copied. Passing the size information leads to a failure suggesting bad arguments passed to DSP. > >> if (err == DSP_UNSUPPORTED_API) { >> dev_info(&cctx->rpdev->dev, >> "Warning: DSP capabilities not supported on domain: %d\n", domain); >> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) >> if (err) >> return err; >> >> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability))) >> + if (copy_to_user(argp, &cap, sizeof(cap))) >> return -EFAULT; >> >> return 0; >> -- >> 2.43.0 >>
On 5/31/2024 3:03 PM, Srinivas Kandagatla wrote: > > > On 30/05/2024 11:20, Ekansh Gupta wrote: >> Incorrect remote arguments are getting passed when requesting for >> capabilities from DSP. Also there is no requirement to update the >> PD type as it might cause problems for any PD other than user PD. >> In addition to this, the collected capability information is not >> getting copied properly to user. Add changes to address these >> problems and get correct DSP capabilities. >> >> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities") >> Cc: stable <stable@kernel.org> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >> --- >> drivers/misc/fastrpc.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index 4028cb96bcf2..61389795f498 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr >> args[0].length = sizeof(dsp_attr_buf_len); >> args[0].fd = -1; >> args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1]; >> - args[1].length = dsp_attr_buf_len; >> + args[1].length = dsp_attr_buf_len * sizeof(uint32_t); > This does not look correct, > > we have allocated buffer of size FASTRPC_MAX_DSP_ATTRIBUTES_LEN which is > already (sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES) > > now this patch multiplies with again sizeof(uint32_t), this is going to send dsp incorrect size for buffer and overrun the buffer size. As the argument passed to this function is number of attributes instead of length, this won't cause another multiplication with (uint32_t). > > > >> args[1].fd = -1; >> - fl->pd = USER_PD; > another patch may be. Sure. > >> return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE, >> FASTRPC_SCALARS(0, 1, 1), args); >> @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, >> if (!dsp_attributes) >> return -ENOMEM; >> - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN); >> + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES); > > You change this again to send FASTRPC_MAX_DSP_ATTRIBUTES instead of FASTRPC_MAX_DSP_ATTRIBUTES_LEN but why? Copying the comment sent to Dmitry's queries: args[0] is expected to carry the information about the total number of attributes to be copied from DSP and not the information about the size to be copied. Passing the size information leads to a failure suggesting bad arguments passed to DSP. > > >> if (err == DSP_UNSUPPORTED_API) { >> dev_info(&cctx->rpdev->dev, >> "Warning: DSP capabilities not supported on domain: %d\n", domain); >> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) >> if (err) >> return err; >> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability))) >> + if (copy_to_user(argp, &cap, sizeof(cap))) > > Why are we copying the full struct here? All that user needs is cap.capability? as argp sent from user during ioctl is the capability structure, the same argp is copied to a local fastrpc_ioctl_capability structure(cap) to get the domain and attribute_id information. Copying just the capability member to argp will cause problem when the user tries to read the capability. While testing the capability, I was observing this failure and it is resolved once we copy the information properly. > > > > --srini > > >> return -EFAULT; >> return 0; >
On Mon, Jun 03, 2024 at 11:45:26AM +0530, Ekansh Gupta wrote: > > On 5/30/2024 4:29 PM, Dmitry Baryshkov wrote: > > On Thu, May 30, 2024 at 03:50:20PM +0530, Ekansh Gupta wrote: > > > Incorrect remote arguments are getting passed when requesting for > > > capabilities from DSP. > > Describe why and how they are incorrect. > > Sure, I'll update this information in the next spin. > > > > > > Also there is no requirement to update the > > > PD type as it might cause problems for any PD other than user PD. > > Also... means that these are two separate issues. There should be two > > separate commits. > > Okay, I'll separate out the PD type change. > > > > > > In addition to this, the collected capability information is not > > > getting copied properly to user. Add changes to address these > > > problems and get correct DSP capabilities. > > > > > > Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities") > > > Cc: stable <stable@kernel.org> > > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > > > --- > > > drivers/misc/fastrpc.c | 7 +++---- > > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > > > index 4028cb96bcf2..61389795f498 100644 > > > --- a/drivers/misc/fastrpc.c > > > +++ b/drivers/misc/fastrpc.c > > > @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr > > > args[0].length = sizeof(dsp_attr_buf_len); > > > args[0].fd = -1; > > > args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1]; > > > - args[1].length = dsp_attr_buf_len; > > > + args[1].length = dsp_attr_buf_len * sizeof(uint32_t); > > As you are skipping first entry, should there be (dsp_attr_buf_len - 1) > > * sizeof(uint32_t). > > This was done in the next patch of the series, I'll bring it here. > > > > > > args[1].fd = -1; > > > - fl->pd = USER_PD; > > > return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE, > > > FASTRPC_SCALARS(0, 1, 1), args); > > > @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, > > > if (!dsp_attributes) > > > return -ENOMEM; > > > - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN); > > > + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES); > > So it looks like the argument was correct. It was passing length, not > > the number of attributes. The only thing to fix is that args[1].length > > should be dsp_attr_buf_len - sizeof(*dsp_attr_buf). > > args[0] is expected to carry the information about the total number of attributes to be copied from DSP > and not the information about the size to be copied. Passing the size information leads to a failure > suggesting bad arguments passed to DSP. AH, so it gets passed twice. As a pointer to u64 (for the number ofh attributes) and as a size for those attributes (via args[1].length). Please explain this in the commit message. > > > > > > if (err == DSP_UNSUPPORTED_API) { > > > dev_info(&cctx->rpdev->dev, > > > "Warning: DSP capabilities not supported on domain: %d\n", domain); > > > @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) > > > if (err) > > > return err; > > > - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability))) > > > + if (copy_to_user(argp, &cap, sizeof(cap))) > > > return -EFAULT; > > > return 0; > > > -- > > > 2.43.0 > > >
On Mon, Jun 03, 2024 at 12:35:40PM +0530, Ekansh Gupta wrote: > > > On 5/31/2024 3:03 PM, Srinivas Kandagatla wrote: > > > > > > On 30/05/2024 11:20, Ekansh Gupta wrote: > >> Incorrect remote arguments are getting passed when requesting for > >> capabilities from DSP. Also there is no requirement to update the > >> PD type as it might cause problems for any PD other than user PD. > >> In addition to this, the collected capability information is not > >> getting copied properly to user. Add changes to address these > >> problems and get correct DSP capabilities. > >> > >> Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities") > >> Cc: stable <stable@kernel.org> > >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > >> --- > >> drivers/misc/fastrpc.c | 7 +++---- > >> 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > >> if (err == DSP_UNSUPPORTED_API) { > >> dev_info(&cctx->rpdev->dev, > >> "Warning: DSP capabilities not supported on domain: %d\n", domain); > >> @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) > >> if (err) > >> return err; > >> - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability))) > >> + if (copy_to_user(argp, &cap, sizeof(cap))) > > > > Why are we copying the full struct here? All that user needs is cap.capability? > as argp sent from user during ioctl is the capability structure, the > same argp is copied to a local fastrpc_ioctl_capability structure(cap) > to get the domain and attribute_id information. Copying just the > capability member to argp will cause problem when the user tries to > read the capability. While testing the capability, I was observing > this failure and it is resolved once we copy the information properly. What kind of failure? Which problems? Why do we need to get all the details from you by asking for more and more details. All this information must be explained in the commit message. [please wrap your lines in a some sensible way, I had to do that for you] > > > > > > > > --srini > > > > > >> return -EFAULT; > >> return 0; > > >
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 4028cb96bcf2..61389795f498 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1700,9 +1700,8 @@ static int fastrpc_get_info_from_dsp(struct fastrpc_user *fl, uint32_t *dsp_attr args[0].length = sizeof(dsp_attr_buf_len); args[0].fd = -1; args[1].ptr = (u64)(uintptr_t)&dsp_attr_buf[1]; - args[1].length = dsp_attr_buf_len; + args[1].length = dsp_attr_buf_len * sizeof(uint32_t); args[1].fd = -1; - fl->pd = USER_PD; return fastrpc_internal_invoke(fl, true, FASTRPC_DSP_UTILITIES_HANDLE, FASTRPC_SCALARS(0, 1, 1), args); @@ -1730,7 +1729,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap, if (!dsp_attributes) return -ENOMEM; - err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES_LEN); + err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES); if (err == DSP_UNSUPPORTED_API) { dev_info(&cctx->rpdev->dev, "Warning: DSP capabilities not supported on domain: %d\n", domain); @@ -1783,7 +1782,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp) if (err) return err; - if (copy_to_user(argp, &cap.capability, sizeof(cap.capability))) + if (copy_to_user(argp, &cap, sizeof(cap))) return -EFAULT; return 0;
Incorrect remote arguments are getting passed when requesting for capabilities from DSP. Also there is no requirement to update the PD type as it might cause problems for any PD other than user PD. In addition to this, the collected capability information is not getting copied properly to user. Add changes to address these problems and get correct DSP capabilities. Fixes: 6c16fd8bdd40 ("misc: fastrpc: Add support to get DSP capabilities") Cc: stable <stable@kernel.org> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> --- drivers/misc/fastrpc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)