| Message ID | 20181220204809.5B78E22783@smtp.opengridcomputing.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [rdma-rc] RDMA/iwcm: Don't copy past the end of dev_name() string | expand |
On Thu, Dec 20, 2018 at 11:37:54AM -0800, Steve Wise wrote: > We now use dev_name(&ib_device->dev) instead of ib_device->name in > iwpm messages. The name field in struct device is a const char *, > where as ib_device->name is a char array of size IB_DEVICE_NAME_MAX, > and it is pre-initialized to zeros. > > Since iw_cm_map() was using memcpy() to copy in the device name, and > copying IWPM_DEVNAME_SIZE bytes, it ends up copying past the device > name string and getting random bytes. This results in iwpmd failing > the REGISTER_PID request from iwcm. Thus port mapping is broken. > > The fix is to initialize the iwpm_dev_data message memory to zeros, > and then strcopy() to avoid copying past the end of the dev_name() string. The fix is to return failure if the name is too long to be held in the iwpm_dev_data... Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@mellanox.com> > Sent: Thursday, December 20, 2018 2:50 PM > To: Steve Wise <swise@opengridcomputing.com> > Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; > chien.tin.tung@intel.com; shiraz.saleem@intel.com > Subject: Re: [PATCH rdma-rc] RDMA/iwcm: Don't copy past the end of > dev_name() string > > On Thu, Dec 20, 2018 at 11:37:54AM -0800, Steve Wise wrote: > > We now use dev_name(&ib_device->dev) instead of ib_device->name in > > iwpm messages. The name field in struct device is a const char *, > > where as ib_device->name is a char array of size IB_DEVICE_NAME_MAX, > > and it is pre-initialized to zeros. > > > > Since iw_cm_map() was using memcpy() to copy in the device name, and > > copying IWPM_DEVNAME_SIZE bytes, it ends up copying past the device > > name string and getting random bytes. This results in iwpmd failing > > the REGISTER_PID request from iwcm. Thus port mapping is broken. > > > > The fix is to initialize the iwpm_dev_data message memory to zeros, > > and then strcopy() to avoid copying past the end of the dev_name() string. > > The fix is to return failure if the name is too long to be held > in the iwpm_dev_data... That would not fix the original memcpy() copying past the end of the struct device name. It wasn't overflowing the iwpm_dev_data, it was copying past the end of the device string and copying garbage. I will enhance iw_cm_map(), if you want, to verify the lengths. but it never overflowed the iwpm_dev_data fields, because the memcpy() commands were bounded by the iwpm_dev_data field lengths. However, if this is to make 4.20-rc, then I recommend we do my proposed change. Is it too late for 4.20? Steve.
On Thu, Dec 20, 2018 at 03:10:14PM -0600, Steve Wise wrote: > > > > From: Jason Gunthorpe <jgg@mellanox.com> > > Sent: Thursday, December 20, 2018 2:50 PM > > To: Steve Wise <swise@opengridcomputing.com> > > Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; > > chien.tin.tung@intel.com; shiraz.saleem@intel.com > > Subject: Re: [PATCH rdma-rc] RDMA/iwcm: Don't copy past the end of > > dev_name() string > > > > On Thu, Dec 20, 2018 at 11:37:54AM -0800, Steve Wise wrote: > > > We now use dev_name(&ib_device->dev) instead of ib_device->name in > > > iwpm messages. The name field in struct device is a const char *, > > > where as ib_device->name is a char array of size IB_DEVICE_NAME_MAX, > > > and it is pre-initialized to zeros. > > > > > > Since iw_cm_map() was using memcpy() to copy in the device name, and > > > copying IWPM_DEVNAME_SIZE bytes, it ends up copying past the device > > > name string and getting random bytes. This results in iwpmd failing > > > the REGISTER_PID request from iwcm. Thus port mapping is broken. > > > > > > The fix is to initialize the iwpm_dev_data message memory to zeros, > > > and then strcopy() to avoid copying past the end of the dev_name() > string. > > > > The fix is to return failure if the name is too long to be held > > in the iwpm_dev_data... > > That would not fix the original memcpy() copying past the end of the struct > device name. It wasn't overflowing the iwpm_dev_data, it was copying past > the end of the device string and copying garbage. Well, you'd use a sensible strncpy and fail if the source string is too long. Handling strings with memcpy is always wrong. Sending a truncated string to userspace is garbage.. > However, if this is to make 4.20-rc, then I recommend we do my proposed > change. Is it too late for 4.20? Yes Jason
> > > On Thu, Dec 20, 2018 at 11:37:54AM -0800, Steve Wise wrote: > > > > We now use dev_name(&ib_device->dev) instead of ib_device->name > in > > > > iwpm messages. The name field in struct device is a const char *, > > > > where as ib_device->name is a char array of size > IB_DEVICE_NAME_MAX, > > > > and it is pre-initialized to zeros. > > > > > > > > Since iw_cm_map() was using memcpy() to copy in the device name, > and > > > > copying IWPM_DEVNAME_SIZE bytes, it ends up copying past the device > > > > name string and getting random bytes. This results in iwpmd failing > > > > the REGISTER_PID request from iwcm. Thus port mapping is broken. > > > > > > > > The fix is to initialize the iwpm_dev_data message memory to zeros, > > > > and then strcopy() to avoid copying past the end of the dev_name() > > string. > > > > > > The fix is to return failure if the name is too long to be held > > > in the iwpm_dev_data... > > > > That would not fix the original memcpy() copying past the end of the struct > > device name. It wasn't overflowing the iwpm_dev_data, it was copying > past > > the end of the device string and copying garbage. > > Well, you'd use a sensible strncpy and fail if the source string is > too long. Handling strings with memcpy is always wrong. That's fine. I just wanted to make sure you were understanding the failure mode. Your initial comment led me to believe you misread the patch. > > Sending a truncated string to userspace is garbage.. > Agreed. > > However, if this is to make 4.20-rc, then I recommend we do my proposed > > change. Is it too late for 4.20? > > Yes Too bad. I'll fix it up and resubmit for for-next with a stable tag. Thanks, Steve.
diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c index ba668d49c751..c8f300d1b4fc 100644 --- a/drivers/infiniband/core/iwcm.c +++ b/drivers/infiniband/core/iwcm.c @@ -502,16 +502,16 @@ static void iw_cm_check_wildcard(struct sockaddr_storage *pm_addr, */ static int iw_cm_map(struct iw_cm_id *cm_id, bool active) { - struct iwpm_dev_data pm_reg_msg; + struct iwpm_dev_data pm_reg_msg = {}; struct iwpm_sa_data pm_msg; int status; cm_id->m_local_addr = cm_id->local_addr; cm_id->m_remote_addr = cm_id->remote_addr; - memcpy(pm_reg_msg.dev_name, dev_name(&cm_id->device->dev), + strncpy(pm_reg_msg.dev_name, dev_name(&cm_id->device->dev), sizeof(pm_reg_msg.dev_name)); - memcpy(pm_reg_msg.if_name, cm_id->device->iwcm->ifname, + strncpy(pm_reg_msg.if_name, cm_id->device->iwcm->ifname, sizeof(pm_reg_msg.if_name)); if (iwpm_register_pid(&pm_reg_msg, RDMA_NL_IWCM) ||
We now use dev_name(&ib_device->dev) instead of ib_device->name in iwpm messages. The name field in struct device is a const char *, where as ib_device->name is a char array of size IB_DEVICE_NAME_MAX, and it is pre-initialized to zeros. Since iw_cm_map() was using memcpy() to copy in the device name, and copying IWPM_DEVNAME_SIZE bytes, it ends up copying past the device name string and getting random bytes. This results in iwpmd failing the REGISTER_PID request from iwcm. Thus port mapping is broken. The fix is to initialize the iwpm_dev_data message memory to zeros, and then strcopy() to avoid copying past the end of the dev_name() string. Fixes: 896de0090a85 ("RDMA/core: Use dev_name instead of ibdev->name") Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/infiniband/core/iwcm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)