Why don't we always check that attr->port_num is valid?

Why don't we always check that attr->port_num is valid?

[Dan Carpenter]

The patch 926a01dc000d: "RDMA/hns: Add QP operations support for
hip08 SoC" from Aug 30, 2017, leads to the following static checker
warning:

	drivers/infiniband/hw/hns/hns_roce_hw_v2.c:2721 hns_roce_v2_modify_qp()
	error: buffer overflow 'hr_dev->iboe.phy_port' 6 <= 255

drivers/infiniband/hw/hns/hns_roce_hw_v2.c
  2716  
  2717          if (attr_mask & IB_QP_MAX_DEST_RD_ATOMIC)
  2718                  hr_qp->resp_depth = attr->max_dest_rd_atomic;
  2719          if (attr_mask & IB_QP_PORT) {
                    ^^^^^^^^^^^^^^^^^^^^^^

  2720                  hr_qp->port = attr->port_num - 1;
                                      ^^^^^^^^^^^^^^^^^^
  2721                  hr_qp->phy_port = hr_dev->iboe.phy_port[hr_qp->port];
                                                                ^^^^^^^^^^^
The warning here is that hns_roce_v2_modify_qp() is called with a user
controlled hr_qp->port and it can be in the 0-255 range.  This is true.
*However* when IB_QP_PORT mask is set then ->port_num is checked.

  2722          }

Here is how it looks in the caller, hns_roce_modify_qp():

drivers/infiniband/hw/hns/hns_roce_qp.c
   757          if ((attr_mask & IB_QP_PORT) &&
   758              (attr->port_num == 0 || attr->port_num > hr_dev->caps.num_ports)) {
   759                  dev_err(dev, "attr port_num invalid.attr->port_num=%d\n",
   760                          attr->port_num);
   761                  goto out;
   762          }

We deliberately allow invalid attr->port_nums if IB_QP_PORT is not set.
Why must we do that?  From a kernel hardening perspective it would be
better to ban invalid values all together...

I'm trying to think of a way to handle this kind of thing in Smatch but
it's like I end up with an explosion of states that end up consuming all
the RAM.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why don't we always check that attr->port_num is valid?

[Jason Gunthorpe]

On Mon, Oct 02, 2017 at 02:34:31PM +0300, Dan Carpenter wrote:

> We deliberately allow invalid attr->port_nums if IB_QP_PORT is not set.
> Why must we do that?  From a kernel hardening perspective it would be
> better to ban invalid values all together...

It is part of the user ABI, so it has to stay that way...

Can some code restructuring bring both things under the same if
somehow?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why don't we always check that attr->port_num is valid?

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On Mon, Oct 02, 2017 at 09:20:33AM -0600, Jason Gunthorpe wrote:
> On Mon, Oct 02, 2017 at 02:34:31PM +0300, Dan Carpenter wrote:
>
> > We deliberately allow invalid attr->port_nums if IB_QP_PORT is not set.
> > Why must we do that?  From a kernel hardening perspective it would be
> > better to ban invalid values all together...
>
> It is part of the user ABI, so it has to stay that way...

Can we pre-process all invalid parameters at the kernel entry points to
ensure that drivers receive clean input?

For example, overwrite attr->port_nums to be zero if IB_QP_PORT is not set.

>
> Can some code restructuring bring both things under the same if
> somehow?
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Why don't we always check that attr->port_num is valid?

[Chien Tin Tung]

On Tue, Oct 03, 2017 at 08:21:59AM +0300, Leon Romanovsky wrote:
> On Mon, Oct 02, 2017 at 09:20:33AM -0600, Jason Gunthorpe wrote:
> > On Mon, Oct 02, 2017 at 02:34:31PM +0300, Dan Carpenter wrote:
> >
> > > We deliberately allow invalid attr->port_nums if IB_QP_PORT is not set.
> > > Why must we do that?  From a kernel hardening perspective it would be
> > > better to ban invalid values all together...
> >
> > It is part of the user ABI, so it has to stay that way...
> 
> Can we pre-process all invalid parameters at the kernel entry points to
> ensure that drivers receive clean input?

Which side?  I hope you meant the kernel side. I certainly wouldn't want
kernel to trust user input...

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why don't we always check that attr->port_num is valid?

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Tue, Oct 03, 2017 at 10:56:06AM -0500, Chien Tin Tung wrote:
> On Tue, Oct 03, 2017 at 08:21:59AM +0300, Leon Romanovsky wrote:
> > On Mon, Oct 02, 2017 at 09:20:33AM -0600, Jason Gunthorpe wrote:
> > > On Mon, Oct 02, 2017 at 02:34:31PM +0300, Dan Carpenter wrote:
> > >
> > > > We deliberately allow invalid attr->port_nums if IB_QP_PORT is not set.
> > > > Why must we do that?  From a kernel hardening perspective it would be
> > > > better to ban invalid values all together...
> > >
> > > It is part of the user ABI, so it has to stay that way...
> >
> > Can we pre-process all invalid parameters at the kernel entry points to
> > ensure that drivers receive clean input?
>
> Which side?  I hope you meant the kernel side. I certainly wouldn't want
> kernel to trust user input...

Yes, Chien, kernel side ("kernel entry points"), it goes without saying.

>
> Chien

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Why don't we always check that attr->port_num is valid?

[Chien Tin Tung]

On Tue, Oct 03, 2017 at 07:50:25PM +0300, Leon Romanovsky wrote:
> On Tue, Oct 03, 2017 at 10:56:06AM -0500, Chien Tin Tung wrote:
> > On Tue, Oct 03, 2017 at 08:21:59AM +0300, Leon Romanovsky wrote:
> > > On Mon, Oct 02, 2017 at 09:20:33AM -0600, Jason Gunthorpe wrote:
> > > > On Mon, Oct 02, 2017 at 02:34:31PM +0300, Dan Carpenter wrote:
> > > >
> > > > > We deliberately allow invalid attr->port_nums if IB_QP_PORT is not set.
> > > > > Why must we do that?  From a kernel hardening perspective it would be
> > > > > better to ban invalid values all together...
> > > >
> > > > It is part of the user ABI, so it has to stay that way...
> > >
> > > Can we pre-process all invalid parameters at the kernel entry points to
> > > ensure that drivers receive clean input?
> >
> > Which side?  I hope you meant the kernel side. I certainly wouldn't want
> > kernel to trust user input...
> 
> Yes, Chien, kernel side ("kernel entry points"), it goes without saying.

Sorry, I had to ask given your past history of confusing user and kernel.

So back to your suggestion, can you give an example of how you would
take care of the check for this particular API, qp_modify?  And how would
you account for differences between protocols IB/iWARP at this level?

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Why don't we always check that attr->port_num is valid?

[Hefty, Sean]

> > > We deliberately allow invalid attr->port_nums if IB_QP_PORT is not
> set.
> > > Why must we do that?  From a kernel hardening perspective it would
> > > be better to ban invalid values all together...
> >
> > It is part of the user ABI, so it has to stay that way...
> 
> Can we pre-process all invalid parameters at the kernel entry points
> to ensure that drivers receive clean input?
> 
> For example, overwrite attr->port_nums to be zero if IB_QP_PORT is not
> set.

I'm not sure this helps much.  The value must still be ignored by the driver, whether it's in range or not.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why don't we always check that attr->port_num is valid?

[Dan Carpenter]

On Tue, Oct 03, 2017 at 07:52:27PM +0000, Hefty, Sean wrote:
> > > > We deliberately allow invalid attr->port_nums if IB_QP_PORT is not
> > set.
> > > > Why must we do that?  From a kernel hardening perspective it would
> > > > be better to ban invalid values all together...
> > >
> > > It is part of the user ABI, so it has to stay that way...
> > 
> > Can we pre-process all invalid parameters at the kernel entry points
> > to ensure that drivers receive clean input?
> > 
> > For example, overwrite attr->port_nums to be zero if IB_QP_PORT is not
> > set.

attr->port_nums is not allowed to be set to zero so that will trigger
another warning.

> 
> I'm not sure this helps much.  The value must still be ignored by the driver, whether it's in range or not.

It would help me a lot to know that the value is always within range
when we call the function.

Smatch is tracking things at the function call level so it says "when
we call this we know that the value comes from the user and is not
trusted".  Which is correct, but not precise.  In an ideal world it
would say, "This comes from the user and is not trusted unless a flag
is set on param 3", but Smatch doesn't break it down that way...

This might be solveable in Smatch...  But generally the problem is that
there is too much data to track the relationships between every variable
in the kernel.  I just break it down at the function call boundaries.
So like, for returns I split them out into success/failure returns and
I say "this is true if the function returns zero", and "these things are
true if the function returns failure".  But for calls I just say "These
things are true when the function is called from this call site".

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html