2023-09-11 23:31 GMT+09:00, Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> Hello Namjae Jeon,
Hi Dan,
>
> The patch e2b76ab8b5c9: "ksmbd: add support for read compound" from
> Aug 29, 2023 (linux-next), leads to the following Smatch static
> checker warning:
>
> fs/smb/server/smb2pdu.c:6329 smb2_read()
> warn: passing freed memory 'aux_payload_buf'
>
> fs/smb/server/smb2pdu.c
> 6283 ksmbd_debug(SMB, "filename %pD, offset %lld, len %zu\n",
> 6284 fp->filp, offset, length);
> 6285
> 6286 aux_payload_buf = kvzalloc(length, GFP_KERNEL);
> 6287 if (!aux_payload_buf) {
> 6288 err = -ENOMEM;
> 6289 goto out;
> 6290 }
> 6291
> 6292 nbytes = ksmbd_vfs_read(work, fp, length, &offset,
> aux_payload_buf);
> 6293 if (nbytes < 0) {
> 6294 err = nbytes;
> 6295 goto out;
> 6296 }
> 6297
> 6298 if ((nbytes == 0 && length != 0) || nbytes < mincount) {
> 6299 kvfree(aux_payload_buf);
> 6300 rsp->hdr.Status = STATUS_END_OF_FILE;
> 6301 smb2_set_err_rsp(work);
> 6302 ksmbd_fd_put(work, fp);
> 6303 return 0;
> 6304 }
> 6305
> 6306 ksmbd_debug(SMB, "nbytes %zu, offset %lld mincount %zu\n",
> 6307 nbytes, offset, mincount);
> 6308
> 6309 if (is_rdma_channel == true) {
> 6310 /* write data to the client using rdma channel */
> 6311 remain_bytes = smb2_read_rdma_channel(work, req,
> 6312
> aux_payload_buf,
> 6313 nbytes);
> 6314 kvfree(aux_payload_buf);
> ^^^^^^^^^^^^^^^^^^^^^^^
> freed
>
> 6315
> 6316 nbytes = 0;
>
> I guess probably it doesn't matter that we're passing a freed variable
> if nbytes is zero.
> But could we also just set "aux_payload_buf = NULL"?
> I am not going to try silence this type of warning in Smatch.
Yes, It isn't a problem... but will fix it to make smatch happy.
Thanks.
>
> 6317 if (remain_bytes < 0) {
> 6318 err = (int)remain_bytes;
> 6319 goto out;
> 6320 }
> 6321 }
> 6322
> 6323 rsp->StructureSize = cpu_to_le16(17);
> 6324 rsp->DataOffset = 80;
> 6325 rsp->Reserved = 0;
> 6326 rsp->DataLength = cpu_to_le32(nbytes);
> 6327 rsp->DataRemaining = cpu_to_le32(remain_bytes);
> 6328 rsp->Flags = 0;
> --> 6329 err = ksmbd_iov_pin_rsp_read(work, (void *)rsp,
> 6330 offsetof(struct smb2_read_rsp,
> Buffer),
> 6331 aux_payload_buf, nbytes);
> ^^^^^^^^^^^^^^^
> Passing free variable.
>
>
> 6332 if (err)
> 6333 goto out;
> 6334 ksmbd_fd_put(work, fp);
> 6335 return 0;
> 6336
> 6337 out:
> 6338 if (err) {
> 6339 if (err == -EISDIR)
> 6340 rsp->hdr.Status =
> STATUS_INVALID_DEVICE_REQUEST;
> 6341 else if (err == -EAGAIN)
> 6342 rsp->hdr.Status =
> STATUS_FILE_LOCK_CONFLICT;
> 6343 else if (err == -ENOENT)
> 6344 rsp->hdr.Status = STATUS_FILE_CLOSED;
> 6345 else if (err == -EACCES)
> 6346 rsp->hdr.Status = STATUS_ACCESS_DENIED;
> 6347 else if (err == -ESHARE)
> 6348 rsp->hdr.Status =
> STATUS_SHARING_VIOLATION;
> 6349 else if (err == -EINVAL)
> 6350 rsp->hdr.Status =
> STATUS_INVALID_PARAMETER;
> 6351 else
> 6352 rsp->hdr.Status = STATUS_INVALID_HANDLE;
> 6353
> 6354 smb2_set_err_rsp(work);
> 6355 }
> 6356 ksmbd_fd_put(work, fp);
> 6357 return err;
> 6358 }
>
> regards,
> dan carpenter
>
