re: drm/vmwgfx: Add VMWare host messaging capability

re: drm/vmwgfx: Add VMWare host messaging capability

[Dan Carpenter]

Hello Sinclair Yeh,

The patch 89da76fde68d: "drm/vmwgfx: Add VMWare host messaging
capability" from Apr 27, 2016, leads to the following static checker
warning:

	drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:303 vmw_recv_msg()
	warn: 'reply' was already freed.

drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
   209  static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
   210                          size_t *msg_len)
   211  {
   212          unsigned long eax, ebx, ecx, edx, si, di, bp;
   213          char *reply;
   214          size_t reply_len;
   215          int retries = 0;
   216  
   217  
   218          *msg_len = 0;
   219          *msg = NULL;
   220  
   221          while (retries < RETRIES) {
   222                  retries++;
   223  
   224                  /* Set up additional parameters */
   225                  si  = channel->cookie_high;
   226                  di  = channel->cookie_low;
   227  
   228                  VMW_PORT(VMW_PORT_CMD_RECVSIZE,
   229                          0, si, di,
   230                          (VMW_HYPERVISOR_PORT | (channel->channel_id << 16)),
   231                          VMW_HYPERVISOR_MAGIC,
   232                          eax, ebx, ecx, edx, si, di);
   233  
   234                  if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0 ||
   235                      (HIGH_WORD(ecx) & MESSAGE_STATUS_HB) == 0) {
   236                          DRM_ERROR("Failed to get reply size\n");
   237                          return -EINVAL;
   238                  }
   239  
   240                  /* No reply available.  This is okay. */
   241                  if ((HIGH_WORD(ecx) & MESSAGE_STATUS_DORECV) == 0)
   242                          return 0;
   243  
   244                  reply_len = ebx;
   245                  reply     = kzalloc(reply_len + 1, GFP_KERNEL);
   246                  if (reply == NULL) {
   247                          DRM_ERROR("Cannot allocate memory for reply\n");
   248                          return -ENOMEM;
   249                  }
   250  
   251  
   252                  /* Receive buffer */
   253                  si  = channel->cookie_high;
   254                  di  = (uintptr_t) reply;
   255                  bp  = channel->cookie_low;
   256  
   257                  VMW_PORT_HB_IN(
   258                          (MESSAGE_STATUS_SUCCESS << 16) | VMW_PORT_CMD_HB_MSG,
   259                          reply_len, si, di,
   260                          VMW_HYPERVISOR_HB_PORT | (channel->channel_id << 16),
   261                          VMW_HYPERVISOR_MAGIC, bp,
   262                          eax, ebx, ecx, edx, si, di);
   263  
   264                  if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) == 0) {
   265                          kfree(reply);
   266  
   267                          if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0) {
   268                                  /* A checkpoint occurred. Retry. */
   269                                  continue;
   270                          }
   271  
   272                          return -EINVAL;
   273                  }
   274  
   275                  reply[reply_len] = '\0';
   276  
   277  
   278                  /* Ack buffer */
   279                  si  = channel->cookie_high;
   280                  di  = channel->cookie_low;
   281  
   282                  VMW_PORT(VMW_PORT_CMD_RECVSTATUS,
   283                          MESSAGE_STATUS_SUCCESS, si, di,
   284                          (VMW_HYPERVISOR_PORT | (channel->channel_id << 16)),
   285                          VMW_HYPERVISOR_MAGIC,
   286                          eax, ebx, ecx, edx, si, di);
   287  
   288                  if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
   289                          kfree(reply);
                                      ^^^^^
Freed here.

   290  
   291                          if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0) {
   292                                  /* A checkpoint occurred. Retry. */
   293                                  continue;
   294                          }
   295  
   296                          return -EINVAL;
   297                  }
   298  
   299                  break;
   300          }

There should be a test here to see if (retries == RETRIES) then return
an error code instead of setting it to a bogus thing.

   301  
   302          *msg_len = reply_len;
   303          *msg     = reply;
   304  
   305          return 0;
   306  }

regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel