* re: ufs: add ioctl interface for query request
@ 2015-04-09 20:17 Dan Carpenter
2015-04-13 10:55 ` Gilad Broner
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2015-04-09 20:17 UTC (permalink / raw)
To: draviv; +Cc: linux-scsi
Hello Dolev Raviv,
The patch 4aca8e8975db: "ufs: add ioctl interface for query request"
from Mar 12, 2015, leads to the following Smatch warning:
drivers/scsi/ufs/ufshcd.c:4386 ufshcd_query_ioctl()
warn: maybe return -EFAULT instead of the bytes remaining?
drivers/scsi/ufs/ufshcd.c
4364 /* copy to user */
4365 err = copy_to_user(buffer, ioctl_data,
4366 sizeof(struct ufs_ioctl_query_data));
4367 if (err)
4368 dev_err(hba->dev, "%s: Failed copying back to user.\n",
4369 __func__);
copy_to/from_user() returns the number of bytes not copied and not an
error code. Printing these error messages in the ioctl means the user
can trigger a DoS by filling up /var/log/messages. They make the code
uglier. We should stop here if the copy fails and goto out_release_mem
otherwise we might end up returning success by mistake.
The normal way to do it is:
if (copy_to_user(buffer, ioctl_data,
sizeof(struct ufs_ioctl_query_data))) {
err = -EFAULT;
goto out_release_mem;
}
4370 err = copy_to_user(buffer + sizeof(struct ufs_ioctl_query_data),
4371 data_ptr, ioctl_data->buf_size);
4372 if (err)
4373 dev_err(hba->dev, "%s: err %d copying back to user.\n",
4374 __func__, err);
4375 goto out_release_mem;
4376
4377 out_einval:
4378 dev_err(hba->dev,
4379 "%s: illegal ufs query ioctl data, opcode 0x%x, idn 0x%x\n",
4380 __func__, ioctl_data->opcode, (unsigned int)ioctl_data->idn);
4381 err = -EINVAL;
4382 out_release_mem:
4383 kfree(ioctl_data);
4384 kfree(desc);
4385 out:
4386 return err;
4387 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* re: ufs: add ioctl interface for query request
2015-04-09 20:17 ufs: add ioctl interface for query request Dan Carpenter
@ 2015-04-13 10:55 ` Gilad Broner
2015-04-13 11:01 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Gilad Broner @ 2015-04-13 10:55 UTC (permalink / raw)
To: Dan Carpenter; +Cc: draviv, linux-scsi
> copy_to/from_user() returns the number of bytes not copied and not an
> error code. Printing these error messages in the ioctl means the user
> can trigger a DoS by filling up /var/log/messages. They make the code
> uglier. We should stop here if the copy fails and goto out_release_mem
> otherwise we might end up returning success by mistake.
Thanks for the comment, I will update the code.
I wanted to clarify about the possibility of a DoS attack:
- Isn't var/log/messages cyclic or size limited in some way?
- What determines that dev_err() prints go to /var/log/messages?
would it still be the case if dev_dbg() was used instead?
Thanks,
Gilad.
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: ufs: add ioctl interface for query request
2015-04-13 10:55 ` Gilad Broner
@ 2015-04-13 11:01 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2015-04-13 11:01 UTC (permalink / raw)
To: Gilad Broner; +Cc: draviv, linux-scsi
On Mon, Apr 13, 2015 at 10:55:58AM -0000, Gilad Broner wrote:
> > copy_to/from_user() returns the number of bytes not copied and not an
> > error code. Printing these error messages in the ioctl means the user
> > can trigger a DoS by filling up /var/log/messages. They make the code
> > uglier. We should stop here if the copy fails and goto out_release_mem
> > otherwise we might end up returning success by mistake.
>
> Thanks for the comment, I will update the code.
> I wanted to clarify about the possibility of a DoS attack:
> - Isn't var/log/messages cyclic or size limited in some way?
Some people limit it some people don't. To be honest, I'm not really
concerned about this, but I use it as an excuse to remove debug
messages.
> - What determines that dev_err() prints go to /var/log/messages?
> would it still be the case if dev_dbg() was used instead?
Normally dev_dbg() message are not printed. But just remove them. They
make the code uglier.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-04-13 11:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-09 20:17 ufs: add ioctl interface for query request Dan Carpenter
2015-04-13 10:55 ` Gilad Broner
2015-04-13 11:01 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.