Page MenuHomeFreeBSD
Differential D7044

Add a mask of optional ptrace() events.
ClosedPublic
Actions

Authored by jhb on Jul 1 2016, 1:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 5, 2:24 AM
Unknown Object (File)
Mar 10 2025, 10:56 PM
Unknown Object (File)
Mar 9 2025, 9:35 PM
Unknown Object (File)
Mar 3 2025, 9:23 PM
Unknown Object (File)
Feb 5 2025, 11:31 PM
Unknown Object (File)
Jan 23 2025, 9:25 PM
Unknown Object (File)
Jan 3 2025, 12:01 PM
Unknown Object (File)
Nov 30 2024, 1:55 AM
View All Files
Subscribers
emaste
imp

Details

Reviewers
kib
Group Reviewers
manpages
Commits
rS302902: Add a mask of optional ptrace() events.
Summary

Add a mask of optional ptrace() events.

ptrace() now stores a mask of optional events in p_ptevents. Currently
this mask is a single integer, but it can be expanded into an array of
integers in the future.

Two new ptrace requests can be used to manipulate the event mask:
PT_GET_EVENT_MASK fetches the current event mask and PT_SET_EVENT_MASK
sets the current event mask.

The current set of events include:

  • PTRACE_EXEC: trace calls to execve().
  • PTRACE_SIG: trace signals.
  • PTRACE_SCE: trace system call entries.
  • PTRACE_SCX: trace syscam call exits.
  • PTRACE_FORK: trace forks and auto-attach to new child processes.
  • PTRACE_LWP: trace LWP events.

The S_PT_SCX and S_PT_SCE events in the procfs p_stops flags have
been replaced by PTRACE_SCE and PTRACE_SCX. PTRACE_FORK replaces
P_FOLLOW_FORK and PTRACE_LWP replaces P2_LWP_EVENTS.

Assuming I can't fit this into 11.0, I may try to request the p_ptevents
slot in the pre-zeroed area at least. Its not the end of the world if
it has to wait though.

Test Plan
  • Run new tests as well as existing ptrace tests.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4515
Build 4567: arc lint + arc unit

Event Timeline

jhb updated this revision to Diff 18051.Jul 1 2016, 1:47 AM
jhb retitled this revision from to Add a mask of optional ptrace() events..
jhb updated this object.
jhb edited the test plan for this revision. (Show Details)
jhb added a reviewer: kib.
Herald added subscribers: emaste, imp. · View Herald TranscriptJul 1 2016, 1:47 AM
kib edited edge metadata.Jul 1 2016, 3:29 AM

Does P_TRACED implies that PTRACE_SIG must be set ? I would expect this, but this is not enforced by the PT_SET_EVENT_MASK interface.

Is P_TRACED flag needed ? What would the behaviour if debugger clears the mask ?

sys/kern/kern_exit.c
341

Why is this needed ?

sys/kern/sys_process.c
1004

You don't check that only supported bits are set in the user supplied mask.

jhb marked an inline comment as done.Jul 1 2016, 5:55 PM
jhb added inline comments.
sys/kern/kern_exit.c
341

I explicitly cleared p_ptevents to 0 in all the places that cleared P_TRACED so that I could just use 'p->p_ptevents & PTRACE_FOO' without also having to check if P_TRACED was still set.

jhb added a comment.Jul 1 2016, 6:15 PM
In D7044#147411, @kib wrote:

Does P_TRACED implies that PTRACE_SIG must be set ? I would expect this, but this is not enforced by the PT_SET_EVENT_MASK interface.

Is P_TRACED flag needed ? What would the behaviour if debugger clears the mask ?

The idea is for P_TRACED to not imply PTRACE_SIG. In theory if PTRACE_SIG is clear, then signals are passed to the debugee transparently without any notice to the debugger. Explicit events like SCE/SCX that call ptracestop() directly will still post events. SIGTRAP due to breakpoints or watchpoints should always register a stop as well regardless of PTRACE_SIG (I suspect I might not be handling that correctly in the current patch, in fact I'm almost certainly not.) It may be that it is just too hard to get that right (and of limited usefulness). If so, I can drop PTRACE_SIG and just leave signal catching always enabled. procfs treats S_SIG as a flag, so I was trying to provide an equivalent level of granularity. It does seem that blocking S_SIG in procfs might also break breakpoints in debuggers though.

jhb updated this revision to Diff 18058.Jul 1 2016, 6:17 PM
jhb edited edge metadata.
  • Check for invalid events.
jhb updated this revision to Diff 18218.Jul 8 2016, 2:26 AM
  • Drop PTRACE_SIG.
  • Add a test for a user signal delivery.
  • Renumber after dropping PTRACE_SIG.
kib accepted this revision.Jul 8 2016, 2:36 AM
kib edited edge metadata.
This revision is now accepted and ready to land.Jul 8 2016, 2:36 AM
jhb updated this revision to Diff 18437.Jul 15 2016, 2:18 AM
jhb edited edge metadata.
  • New child processes should not inherit the event mask.
  • Update the manpage for the event mask.
This revision now requires review to proceed.Jul 15 2016, 2:18 AM
Herald added a reviewer: manpages. · View Herald TranscriptJul 15 2016, 2:18 AM
kib accepted this revision.Jul 15 2016, 2:53 AM
kib edited edge metadata.
kib added inline comments.
lib/libc/sys/ptrace.2
152

'kernel-scheduled threads' ?

155

lowercase N in new.

'Stop _and_ report an event. ' Initially I read it as the statement that PL_FLAG_BORN reporting is not performed anymore.

sys/kern/sys_process.c
659

A comment stating that uap->data is sanity checked elsewhere could be useful.

This revision is now accepted and ready to land.Jul 15 2016, 2:53 AM
jhb updated this revision to Diff 18470.Jul 15 2016, 3:20 PM
jhb edited edge metadata.
jhb marked an inline comment as done.
  • Note that uap->data is validated in kern_ptrace().
  • Suggestions from Konstantin.
This revision now requires review to proceed.Jul 15 2016, 3:20 PM
Herald edited edge metadata. · View Herald TranscriptJul 15 2016, 3:20 PM
jhb marked an inline comment as done.Jul 15 2016, 3:22 PM
jhb added inline comments.
lib/libc/sys/ptrace.2
152

Most places I'm familiar with (textbooks, etc.) just use "kernel threads" for LWPs (vs "user threads" for scheduler activations / libc_r, etc.).

jhb mentioned this in rS302900: Add a test for user signal delivery..Jul 15 2016, 3:28 PM
Closed by commit rS302902: Add a mask of optional ptrace() events. (authored by jhb). · Explain WhyJul 15 2016, 3:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
lib/
libc/
sys/
207 lines
sys/
kern/
4 lines
2 lines
9 lines
3 lines
5 lines
4 lines
75 lines
sys/
4 lines
20 lines
tests/
sys/
kern/
198 lines

Diff 18437

View Options

lib/libc/sys/ptrace.2

Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
char *data = (char *)ifreqp->ifr_data; char tmpdata;
char *data;
void *info = NULL; void *info = NULL;
int retValue = EINVAL; int retValue = EINVAL;
tmpdata = fubyte(ifreqp->ifr_data);
if (tmpdata == -1)
return (EFAULT);
data = &tmpdata;
switch(*data) { switch(*data) {
jhbUnsubmitted
Not Done
Inline Actions

If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:

char query;
...

query = fubyte(ifreqp->ifr_data);
if (query == -1)
     return (EFAULT);

switch (query) {

Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that)

jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
brooksAuthorUnsubmitted
Not Done
Inline Actions

data is used later or I would have done that. We don't currently have fuebyte()

brooks: `data` is used later or I would have done that. We don't currently have fuebyte()
case XGE_QUERY_STATS: case XGE_QUERY_STATS:
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_stats_hw(lldev->devh, status = xge_hal_stats_hw(lldev->devh,
(xge_hal_stats_hw_info_t **)&info); (xge_hal_stats_hw_info_t **)&info);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(info, ifreqp->ifr_data, if(copyout(info, ifreqp->ifr_data,
sizeof(xge_hal_stats_hw_info_t)) == 0) sizeof(xge_hal_stats_hw_info_t)) == 0)
▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines if(copyout(&lldev->driver_stats, ifreqp->ifr_data,
sizeof(int)) == 0) sizeof(int)) == 0)
retValue = 0; retValue = 0;
break; break;
case XGE_SET_BUFFER_MODE_1: case XGE_SET_BUFFER_MODE_1:
case XGE_SET_BUFFER_MODE_2: case XGE_SET_BUFFER_MODE_2:
case XGE_SET_BUFFER_MODE_5: case XGE_SET_BUFFER_MODE_5:
*data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N'; *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
jhbUnsubmitted
Done
Inline Actions

So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout().

jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would…
retValue = 0; retValue = 0;
break; break;
default: default:
xge_trace(XGE_TRACE, "Nothing is matching"); xge_trace(XGE_TRACE, "Nothing is matching");
retValue = ENOTTY; retValue = ENOTTY;
break; break;
} }
return retValue; return retValue;
} }
/** /**
* xge_ioctl_registers * xge_ioctl_registers
* IOCTL to get registers * IOCTL to get registers
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_register_t *data = (xge_register_t *)ifreqp->ifr_data; xge_register_t tmpdata;
xge_register_t *data;
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
int retValue = EINVAL, offset = 0, index = 0; int retValue = EINVAL, offset = 0, index = 0;
int error;
u64 val64 = 0; u64 val64 = 0;
error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));
if (error != 0)
return (error);
data = &tmpdata;
/* Reading a register */ /* Reading a register */
if(strcmp(data->option, "-r") == 0) { if(strcmp(data->option, "-r") == 0) {
data->value = 0x0000; data->value = 0x0000;
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
&data->value); &data->value);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
jhbUnsubmitted
Not Done
Inline Actions

The fact that it does copyout() already makes this function even more amusing.

jhb: The fact that it does copyout() already makes this function even more amusing.
brooksAuthorUnsubmitted
Not Done
Inline Actions

The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first.

This driver is best fixed with svn rm, but I'm changing ifr_data use so needed to do a little fixing.

brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
retValue = 0; retValue = 0;
} }
} }
/* Writing to a register */ /* Writing to a register */
else if(strcmp(data->option, "-w") == 0) { else if(strcmp(data->option, "-w") == 0) {
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
data->value); data->value);
▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines
View Options

sys/kern/kern_exec.c

Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
char *data = (char *)ifreqp->ifr_data; char tmpdata;
char *data;
void *info = NULL; void *info = NULL;
int retValue = EINVAL; int retValue = EINVAL;
tmpdata = fubyte(ifreqp->ifr_data);
if (tmpdata == -1)
return (EFAULT);
data = &tmpdata;
switch(*data) { switch(*data) {
jhbUnsubmitted
Not Done
Inline Actions

If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:

char query;
...

query = fubyte(ifreqp->ifr_data);
if (query == -1)
     return (EFAULT);

switch (query) {

Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that)

jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
brooksAuthorUnsubmitted
Not Done
Inline Actions

data is used later or I would have done that. We don't currently have fuebyte()

brooks: `data` is used later or I would have done that. We don't currently have fuebyte()
case XGE_QUERY_STATS: case XGE_QUERY_STATS:
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_stats_hw(lldev->devh, status = xge_hal_stats_hw(lldev->devh,
(xge_hal_stats_hw_info_t **)&info); (xge_hal_stats_hw_info_t **)&info);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(info, ifreqp->ifr_data, if(copyout(info, ifreqp->ifr_data,
sizeof(xge_hal_stats_hw_info_t)) == 0) sizeof(xge_hal_stats_hw_info_t)) == 0)
▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines if(copyout(&lldev->driver_stats, ifreqp->ifr_data,
sizeof(int)) == 0) sizeof(int)) == 0)
retValue = 0; retValue = 0;
break; break;
case XGE_SET_BUFFER_MODE_1: case XGE_SET_BUFFER_MODE_1:
case XGE_SET_BUFFER_MODE_2: case XGE_SET_BUFFER_MODE_2:
case XGE_SET_BUFFER_MODE_5: case XGE_SET_BUFFER_MODE_5:
*data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N'; *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
jhbUnsubmitted
Done
Inline Actions

So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout().

jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would…
retValue = 0; retValue = 0;
break; break;
default: default:
xge_trace(XGE_TRACE, "Nothing is matching"); xge_trace(XGE_TRACE, "Nothing is matching");
retValue = ENOTTY; retValue = ENOTTY;
break; break;
} }
return retValue; return retValue;
} }
/** /**
* xge_ioctl_registers * xge_ioctl_registers
* IOCTL to get registers * IOCTL to get registers
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_register_t *data = (xge_register_t *)ifreqp->ifr_data; xge_register_t tmpdata;
xge_register_t *data;
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
int retValue = EINVAL, offset = 0, index = 0; int retValue = EINVAL, offset = 0, index = 0;
int error;
u64 val64 = 0; u64 val64 = 0;
error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));
if (error != 0)
return (error);
data = &tmpdata;
/* Reading a register */ /* Reading a register */
if(strcmp(data->option, "-r") == 0) { if(strcmp(data->option, "-r") == 0) {
data->value = 0x0000; data->value = 0x0000;
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
&data->value); &data->value);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
jhbUnsubmitted
Not Done
Inline Actions

The fact that it does copyout() already makes this function even more amusing.

jhb: The fact that it does copyout() already makes this function even more amusing.
brooksAuthorUnsubmitted
Not Done
Inline Actions

The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first.

This driver is best fixed with svn rm, but I'm changing ifr_data use so needed to do a little fixing.

brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
retValue = 0; retValue = 0;
} }
} }
/* Writing to a register */ /* Writing to a register */
else if(strcmp(data->option, "-w") == 0) { else if(strcmp(data->option, "-w") == 0) {
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
data->value); data->value);
▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines
View Options

sys/kern/kern_exit.c

Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
char *data = (char *)ifreqp->ifr_data; char tmpdata;
char *data;
void *info = NULL; void *info = NULL;
int retValue = EINVAL; int retValue = EINVAL;
tmpdata = fubyte(ifreqp->ifr_data);
if (tmpdata == -1)
return (EFAULT);
data = &tmpdata;
switch(*data) { switch(*data) {
jhbUnsubmitted
Not Done
Inline Actions

If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:

char query;
...

query = fubyte(ifreqp->ifr_data);
if (query == -1)
     return (EFAULT);

switch (query) {

Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that)

jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
brooksAuthorUnsubmitted
Not Done
Inline Actions

data is used later or I would have done that. We don't currently have fuebyte()

brooks: `data` is used later or I would have done that. We don't currently have fuebyte()
case XGE_QUERY_STATS: case XGE_QUERY_STATS:
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_stats_hw(lldev->devh, status = xge_hal_stats_hw(lldev->devh,
(xge_hal_stats_hw_info_t **)&info); (xge_hal_stats_hw_info_t **)&info);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(info, ifreqp->ifr_data, if(copyout(info, ifreqp->ifr_data,
sizeof(xge_hal_stats_hw_info_t)) == 0) sizeof(xge_hal_stats_hw_info_t)) == 0)
▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines if(copyout(&lldev->driver_stats, ifreqp->ifr_data,
sizeof(int)) == 0) sizeof(int)) == 0)
retValue = 0; retValue = 0;
break; break;
case XGE_SET_BUFFER_MODE_1: case XGE_SET_BUFFER_MODE_1:
case XGE_SET_BUFFER_MODE_2: case XGE_SET_BUFFER_MODE_2:
case XGE_SET_BUFFER_MODE_5: case XGE_SET_BUFFER_MODE_5:
*data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N'; *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
jhbUnsubmitted
Done
Inline Actions

So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout().

jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would…
retValue = 0; retValue = 0;
break; break;
default: default:
xge_trace(XGE_TRACE, "Nothing is matching"); xge_trace(XGE_TRACE, "Nothing is matching");
retValue = ENOTTY; retValue = ENOTTY;
break; break;
} }
return retValue; return retValue;
} }
/** /**
* xge_ioctl_registers * xge_ioctl_registers
* IOCTL to get registers * IOCTL to get registers
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_register_t *data = (xge_register_t *)ifreqp->ifr_data; xge_register_t tmpdata;
xge_register_t *data;
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
int retValue = EINVAL, offset = 0, index = 0; int retValue = EINVAL, offset = 0, index = 0;
int error;
u64 val64 = 0; u64 val64 = 0;
error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));
if (error != 0)
return (error);
data = &tmpdata;
/* Reading a register */ /* Reading a register */
if(strcmp(data->option, "-r") == 0) { if(strcmp(data->option, "-r") == 0) {
data->value = 0x0000; data->value = 0x0000;
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
&data->value); &data->value);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
jhbUnsubmitted
Not Done
Inline Actions

The fact that it does copyout() already makes this function even more amusing.

jhb: The fact that it does copyout() already makes this function even more amusing.
brooksAuthorUnsubmitted
Not Done
Inline Actions

The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first.

This driver is best fixed with svn rm, but I'm changing ifr_data use so needed to do a little fixing.

brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
retValue = 0; retValue = 0;
} }
} }
/* Writing to a register */ /* Writing to a register */
else if(strcmp(data->option, "-w") == 0) { else if(strcmp(data->option, "-w") == 0) {
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
data->value); data->value);
▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines
View Options

sys/kern/kern_fork.c

Show First 20 Lines • Show All 1,358 Lines • ▼ Show 20 Lines
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
char *data = (char *)ifreqp->ifr_data; char tmpdata;
char *data;
void *info = NULL; void *info = NULL;
int retValue = EINVAL; int retValue = EINVAL;
tmpdata = fubyte(ifreqp->ifr_data);
if (tmpdata == -1)
return (EFAULT);
data = &tmpdata;
switch(*data) { switch(*data) {
jhbUnsubmitted
Not Done
Inline Actions

If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone trying to use additional bytes and instead do something like:

char query;
...

query = fubyte(ifreqp->ifr_data);
if (query == -1)
     return (EFAULT);

switch (query) {

Also, I wonder if we should prefer fuebyte() in new code instead? (Maybe kib@ has an opinion on that)

jhb: If this is the only use, I would actually prefer to kill 'data' entirely to avoid someone…
brooksAuthorUnsubmitted
Not Done
Inline Actions

data is used later or I would have done that. We don't currently have fuebyte()

brooks: `data` is used later or I would have done that. We don't currently have fuebyte()
case XGE_QUERY_STATS: case XGE_QUERY_STATS:
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_stats_hw(lldev->devh, status = xge_hal_stats_hw(lldev->devh,
(xge_hal_stats_hw_info_t **)&info); (xge_hal_stats_hw_info_t **)&info);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(info, ifreqp->ifr_data, if(copyout(info, ifreqp->ifr_data,
sizeof(xge_hal_stats_hw_info_t)) == 0) sizeof(xge_hal_stats_hw_info_t)) == 0)
▲ Show 20 Lines • Show All 112 Lines • ▼ Show 20 Lines if(copyout(&lldev->driver_stats, ifreqp->ifr_data,
sizeof(int)) == 0) sizeof(int)) == 0)
retValue = 0; retValue = 0;
break; break;
case XGE_SET_BUFFER_MODE_1: case XGE_SET_BUFFER_MODE_1:
case XGE_SET_BUFFER_MODE_2: case XGE_SET_BUFFER_MODE_2:
case XGE_SET_BUFFER_MODE_5: case XGE_SET_BUFFER_MODE_5:
*data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N'; *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)
jhbUnsubmitted
Done
Inline Actions

So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would probably fix this by not abusing data but just using a local 'char mode' and passing '&mode' to copyout().

jhb: So this is the only place that uses 'data' and it is wrong (sizeof is wrong). I would…
retValue = 0; retValue = 0;
break; break;
default: default:
xge_trace(XGE_TRACE, "Nothing is matching"); xge_trace(XGE_TRACE, "Nothing is matching");
retValue = ENOTTY; retValue = ENOTTY;
break; break;
} }
return retValue; return retValue;
} }
/** /**
* xge_ioctl_registers * xge_ioctl_registers
* IOCTL to get registers * IOCTL to get registers
* *
* @lldev Per-adapter data * @lldev Per-adapter data
* @ifreqp Interface request * @ifreqp Interface request
*/ */
int int
xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp) xge_ioctl_registers(xge_lldev_t *lldev, struct ifreq *ifreqp)
{ {
xge_register_t *data = (xge_register_t *)ifreqp->ifr_data; xge_register_t tmpdata;
xge_register_t *data;
xge_hal_status_e status = XGE_HAL_OK; xge_hal_status_e status = XGE_HAL_OK;
int retValue = EINVAL, offset = 0, index = 0; int retValue = EINVAL, offset = 0, index = 0;
int error;
u64 val64 = 0; u64 val64 = 0;
error = copyin(ifreqp->ifr_data, &tmpdata, sizeof(tmpdata));
if (error != 0)
return (error);
data = &tmpdata;
/* Reading a register */ /* Reading a register */
if(strcmp(data->option, "-r") == 0) { if(strcmp(data->option, "-r") == 0) {
data->value = 0x0000; data->value = 0x0000;
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_read(lldev->devh, 0, data->offset,
&data->value); &data->value);
mtx_unlock(&lldev->mtx_drv); mtx_unlock(&lldev->mtx_drv);
if(status == XGE_HAL_OK) { if(status == XGE_HAL_OK) {
if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0) if(copyout(data, ifreqp->ifr_data, sizeof(xge_register_t)) == 0)
jhbUnsubmitted
Not Done
Inline Actions

The fact that it does copyout() already makes this function even more amusing.

jhb: The fact that it does copyout() already makes this function even more amusing.
brooksAuthorUnsubmitted
Not Done
Inline Actions

The strcmp() cases will certainly not trigger in most cases because option is a char[2] followed by an int64_t so there's garbage in the pad unless the did a bzero first.

This driver is best fixed with svn rm, but I'm changing ifr_data use so needed to do a little fixing.

brooks: The strcmp() cases will certainly not trigger in most cases because option is a char[2]…
retValue = 0; retValue = 0;
} }
} }
/* Writing to a register */ /* Writing to a register */
else if(strcmp(data->option, "-w") == 0) { else if(strcmp(data->option, "-w") == 0) {
mtx_lock(&lldev->mtx_drv); mtx_lock(&lldev->mtx_drv);
status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset, status = xge_hal_mgmt_reg_write(lldev->devh, 0, data->offset,
data->value); data->value);
▲ Show 20 Lines • Show All 1,982 Lines • Show Last 20 Lines
View Options

sys/kern/kern_sig.c

Loading...
View Options

sys/kern/kern_thr.c

Loading...
View Options

sys/kern/subr_syscall.c

Loading...
View Options

sys/kern/sys_process.c

Loading...
View Options

sys/sys/proc.h

Loading...
View Options

sys/sys/ptrace.h

Loading...
View Options

tests/sys/kern/ptrace_test.c

Loading...