| Message ID | 20220628200427.601714-1-vishal.l.verma@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/mbox: Fix missing variable payload checks in cmd size validation | expand |
On Tue, Jun 28, 2022 at 01:04:27PM -0700, Vishal Verma wrote: > The conversion of command sizes to unsigned missed a couple of checks > against variable size payloads during command validation, which made all > variable payload commands unconditionally fail. Add the checks back using > the new CXL_VARIABLE_PAYLOAD scheme. > > Reported-by: Abhi Cs <abhi.cs@intel.com> > Fixes: 26f89535a5bb ("cxl/mbox: Use type __u32 for mailbox payload sizes") > Cc: <stable@vger.kernel.org> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> with one caveat below... Reviewed-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/mbox.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 40e3ccb2bf3e..d929b89d12a7 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -355,12 +355,14 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, > return -EBUSY; > > /* Check the input buffer is the expected size */ > - if (info->size_in != send_cmd->in.size) > - return -ENOMEM; > + if (info->size_in != CXL_VARIABLE_PAYLOAD) > + if (info->size_in != send_cmd->in.size) > + return -ENOMEM; We can leave it to Dan to arbitrate, but I don't think nested if's without brackets follow kernel coding style. However, Dan didn't like my nested if's with brackets either. He'd prefer using && > > /* Check the output buffer is at least large enough */ > - if (send_cmd->out.size < info->size_out) > - return -ENOMEM; > + if (info->size_out != CXL_VARIABLE_PAYLOAD) > + if (send_cmd->out.size < info->size_out) > + return -ENOMEM; > > *mem_cmd = (struct cxl_mem_command) { > .info = { > > base-commit: 1985cf58850562e4b960e19d46f0d8f19d6c7cbd > -- > 2.36.1 >
On Tue, 2022-06-28 at 14:42 -0700, Alison Schofield wrote: > On Tue, Jun 28, 2022 at 01:04:27PM -0700, Vishal Verma wrote: > > The conversion of command sizes to unsigned missed a couple of > > checks > > against variable size payloads during command validation, which > > made all > > variable payload commands unconditionally fail. Add the checks back > > using > > the new CXL_VARIABLE_PAYLOAD scheme. > > > > Reported-by: Abhi Cs <abhi.cs@intel.com> > > Fixes: 26f89535a5bb ("cxl/mbox: Use type __u32 for mailbox payload > > sizes") > > Cc: <stable@vger.kernel.org> > > Cc: Ira Weiny <ira.weiny@intel.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Cc: Alison Schofield <alison.schofield@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > with one caveat below... > Reviewed-by: Alison Schofield <alison.schofield@intel.com> Thanks for the review! > > --- > > drivers/cxl/core/mbox.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > index 40e3ccb2bf3e..d929b89d12a7 100644 > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > @@ -355,12 +355,14 @@ static int cxl_to_mem_cmd(struct > > cxl_mem_command *mem_cmd, > > return -EBUSY; > > > > /* Check the input buffer is the expected size */ > > - if (info->size_in != send_cmd->in.size) > > - return -ENOMEM; > > + if (info->size_in != CXL_VARIABLE_PAYLOAD) > > + if (info->size_in != send_cmd->in.size) > > + return -ENOMEM; > > We can leave it to Dan to arbitrate, but I don't think nested > if's without brackets follow kernel coding style. > > However, Dan didn't like my nested if's with brackets either. > He'd prefer using && Ha funny - I had && originally, but then I spotted nested if () a few lines above in the same file in cxl_mbox_send_cmd(), and switched to the same style :) I'd be happy to change to &&. > > > > > /* Check the output buffer is at least large enough */ > > - if (send_cmd->out.size < info->size_out) > > - return -ENOMEM; > > + if (info->size_out != CXL_VARIABLE_PAYLOAD) > > + if (send_cmd->out.size < info->size_out) > > + return -ENOMEM; > > > > *mem_cmd = (struct cxl_mem_command) { > > .info = { > > > > base-commit: 1985cf58850562e4b960e19d46f0d8f19d6c7cbd > > -- > > 2.36.1 > >
Vishal Verma wrote: > The conversion of command sizes to unsigned missed a couple of checks > against variable size payloads during command validation, which made all > variable payload commands unconditionally fail. Add the checks back using > the new CXL_VARIABLE_PAYLOAD scheme. Ah, looks good. Need to get label read/write regression test into cxl-cli post-haste. Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Verma, Vishal L wrote: > On Tue, 2022-06-28 at 14:42 -0700, Alison Schofield wrote: > > On Tue, Jun 28, 2022 at 01:04:27PM -0700, Vishal Verma wrote: > > > The conversion of command sizes to unsigned missed a couple of > > > checks > > > against variable size payloads during command validation, which > > > made all > > > variable payload commands unconditionally fail. Add the checks back > > > using > > > the new CXL_VARIABLE_PAYLOAD scheme. > > > > > > Reported-by: Abhi Cs <abhi.cs@intel.com> > > > Fixes: 26f89535a5bb ("cxl/mbox: Use type __u32 for mailbox payload > > > sizes") > > > Cc: <stable@vger.kernel.org> > > > Cc: Ira Weiny <ira.weiny@intel.com> > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > Cc: Alison Schofield <alison.schofield@intel.com> > > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > > > with one caveat below... > > Reviewed-by: Alison Schofield <alison.schofield@intel.com> > > Thanks for the review! > > > > > --- > > > drivers/cxl/core/mbox.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > > index 40e3ccb2bf3e..d929b89d12a7 100644 > > > --- a/drivers/cxl/core/mbox.c > > > +++ b/drivers/cxl/core/mbox.c > > > @@ -355,12 +355,14 @@ static int cxl_to_mem_cmd(struct > > > cxl_mem_command *mem_cmd, > > > return -EBUSY; > > > > > > /* Check the input buffer is the expected size */ > > > - if (info->size_in != send_cmd->in.size) > > > - return -ENOMEM; > > > + if (info->size_in != CXL_VARIABLE_PAYLOAD) > > > + if (info->size_in != send_cmd->in.size) > > > + return -ENOMEM; > > > > We can leave it to Dan to arbitrate, but I don't think nested > > if's without brackets follow kernel coding style. > > > > However, Dan didn't like my nested if's with brackets either. > > He'd prefer using && > > Ha funny - I had && originally, but then I spotted nested if () a few > lines above in the same file in cxl_mbox_send_cmd(), and switched to > the same style :) > > I'd be happy to change to &&. Yeah, lets go that route. /me thanks Alison for catching it.
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 40e3ccb2bf3e..d929b89d12a7 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -355,12 +355,14 @@ static int cxl_to_mem_cmd(struct cxl_mem_command *mem_cmd, return -EBUSY; /* Check the input buffer is the expected size */ - if (info->size_in != send_cmd->in.size) - return -ENOMEM; + if (info->size_in != CXL_VARIABLE_PAYLOAD) + if (info->size_in != send_cmd->in.size) + return -ENOMEM; /* Check the output buffer is at least large enough */ - if (send_cmd->out.size < info->size_out) - return -ENOMEM; + if (info->size_out != CXL_VARIABLE_PAYLOAD) + if (send_cmd->out.size < info->size_out) + return -ENOMEM; *mem_cmd = (struct cxl_mem_command) { .info = {
The conversion of command sizes to unsigned missed a couple of checks against variable size payloads during command validation, which made all variable payload commands unconditionally fail. Add the checks back using the new CXL_VARIABLE_PAYLOAD scheme. Reported-by: Abhi Cs <abhi.cs@intel.com> Fixes: 26f89535a5bb ("cxl/mbox: Use type __u32 for mailbox payload sizes") Cc: <stable@vger.kernel.org> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Alison Schofield <alison.schofield@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/cxl/core/mbox.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) base-commit: 1985cf58850562e4b960e19d46f0d8f19d6c7cbd