| Message ID | 1381184340-11190-1-git-send-email-geyslan@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2013-10-07 at 19:19 -0300, Geyslan G. Bem wrote: > Changes the sign type to unsigned, avoiding the possibility of > wrap when ORing the p9 or unix bit modes. [] > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c [] > @@ -144,7 +144,7 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses, > else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses)) > && (v9ses->nodev == 0)) { > char type = 0, ext[32]; > - int major = -1, minor = -1; > + u32 major = 0, minor = 0; > > strlcpy(ext, stat->extension, sizeof(ext)); > sscanf(ext, "%c %u %u", &type, &major, &minor); This bit changes the MKDEV below it. I would think that the sscanf return should be checked for 3 and maybe MKDEV should not be constructed when it's not. ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
Joe,
Thank you for reply.
What do you think about:
strncpy(ext, stat->extension, sizeof(ext));
+ if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
+ p9_debug(P9_DEBUG_ERROR,
+ "It's necessary define type, major
and minor values when using P9_DMDEVICE");
+ return res;
+ }
switch (type) {
case 'c':
res |= S_IFCHR;
break;
...
*rdev = MKDEV(major, minor);
Geyslan Gregório Bem
hackingbits.com
2013/10/7 Joe Perches <joe@perches.com>:
> On Mon, 2013-10-07 at 19:19 -0300, Geyslan G. Bem wrote:
>> Changes the sign type to unsigned, avoiding the possibility of
>> wrap when ORing the p9 or unix bit modes.
> []
>> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> []
>> @@ -144,7 +144,7 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
>> else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
>> && (v9ses->nodev == 0)) {
>> char type = 0, ext[32];
>> - int major = -1, minor = -1;
>> + u32 major = 0, minor = 0;
>>
>> strlcpy(ext, stat->extension, sizeof(ext));
>> sscanf(ext, "%c %u %u", &type, &major, &minor);
>
> This bit changes the MKDEV below it.
>
> I would think that the sscanf return should be
> checked for 3 and maybe MKDEV should not be
> constructed when it's not.
>
>
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote: > Joe, > > Thank you for reply. > > What do you think about: > > strncpy(ext, stat->extension, sizeof(ext)); > + if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) { > + p9_debug(P9_DEBUG_ERROR, > + "It's necessary define type, major > and minor values when using P9_DMDEVICE"); > + return res; > + } > switch (type) { > case 'c': > res |= S_IFCHR; > break; > ... > *rdev = MKDEV(major, minor); I think the plan 9 folk should figure out what's right. ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
Joe, Nice, I'll wait their reply, there are other p9 patches that I have already sent and am awaiting Eric's. Thank you again. Geyslan Gregório Bem hackingbits.com 2013/10/7 Joe Perches <joe@perches.com>: > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote: >> Joe, >> >> Thank you for reply. >> >> What do you think about: >> >> strncpy(ext, stat->extension, sizeof(ext)); >> + if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) { >> + p9_debug(P9_DEBUG_ERROR, >> + "It's necessary define type, major >> and minor values when using P9_DMDEVICE"); >> + return res; >> + } >> switch (type) { >> case 'c': >> res |= S_IFCHR; >> break; >> ... >> *rdev = MKDEV(major, minor); > > I think the plan 9 folk should figure out what's right. > > ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
Please resubmit a clean patch which includes the check of sscanf for
exactly the correct number of arguments and handles errors properly in
other cases. That last bit may be a bit problematic since right now the
only errors are prints and we seem to be otherwise silently failing. Of
course, looks like nothing else is checking return values from that
function for error. We could set rdev to an ERR_PTR(-EIO), and then go
through and do a check in all the places which matter (looks like there are
a few places where rdev just gets discarded -- might even be a good idea to
not parse rdev unless we need to by passing NULL to p9mode2unixmode.
All in all, its a corner case which is only likely with a broken server,
but the full clean up would seem to be:
a) switch to u32's
b) pass NULL when rdev just gets discarded and don't bother parsing when
it is
c) check the sscanf return validity
d) on error set ERR_PTR in rdev and check on return before proceeding
That's a lot of cleanup, I'll add it to my work queue if you don't have
time to rework your patch.
For the other patches, anyone you didn't see a response from me on today is
being pulled into my for-next queue. Thanks for the cleanups.
-eric
On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem <geyslan@gmail.com>wrote:
> Joe,
>
> Nice, I'll wait their reply, there are other p9 patches that I have
> already sent and am awaiting Eric's.
>
> Thank you again.
>
> Geyslan Gregório Bem
> hackingbits.com
>
>
> 2013/10/7 Joe Perches <joe@perches.com>:
> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
> >> Joe,
> >>
> >> Thank you for reply.
> >>
> >> What do you think about:
> >>
> >> strncpy(ext, stat->extension, sizeof(ext));
> >> + if (sscanf(ext, "%c %u %u", &type, &major, &minor) <
> 3) {
> >> + p9_debug(P9_DEBUG_ERROR,
> >> + "It's necessary define type, major
> >> and minor values when using P9_DMDEVICE");
> >> + return res;
> >> + }
> >> switch (type) {
> >> case 'c':
> >> res |= S_IFCHR;
> >> break;
> >> ...
> >> *rdev = MKDEV(major, minor);
> >
> > I think the plan 9 folk should figure out what's right.
> >
> >
>
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
2013/10/20 Eric Van Hensbergen <ericvh@gmail.com>: > Please resubmit a clean patch which includes the check of sscanf for exactly > the correct number of arguments and handles errors properly in other cases. > That last bit may be a bit problematic since right now the only errors are > prints and we seem to be otherwise silently failing. Of course, looks like > nothing else is checking return values from that function for error. We > could set rdev to an ERR_PTR(-EIO), and then go through and do a check in > all the places which matter (looks like there are a few places where rdev > just gets discarded -- might even be a good idea to not parse rdev unless we > need to by passing NULL to p9mode2unixmode. > > All in all, its a corner case which is only likely with a broken server, but > the full clean up would seem to be: > a) switch to u32's > b) pass NULL when rdev just gets discarded and don't bother parsing when > it is > c) check the sscanf return validity > d) on error set ERR_PTR in rdev and check on return before proceeding > > That's a lot of cleanup, I'll add it to my work queue if you don't have time > to rework your patch. > Eric, I would like to try with your guidance. > For the other patches, anyone you didn't see a response from me on today is > being pulled into my for-next queue. Thanks for the cleanups. > > -eric Thanks for accept them. > > > > > On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem <geyslan@gmail.com> > wrote: >> >> Joe, >> >> Nice, I'll wait their reply, there are other p9 patches that I have >> already sent and am awaiting Eric's. >> >> Thank you again. >> >> Geyslan Gregório Bem >> hackingbits.com >> >> >> 2013/10/7 Joe Perches <joe@perches.com>: >> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote: >> >> Joe, >> >> >> >> Thank you for reply. >> >> >> >> What do you think about: >> >> >> >> strncpy(ext, stat->extension, sizeof(ext)); >> >> + if (sscanf(ext, "%c %u %u", &type, &major, &minor) < >> >> 3) { >> >> + p9_debug(P9_DEBUG_ERROR, >> >> + "It's necessary define type, major >> >> and minor values when using P9_DMDEVICE"); >> >> + return res; >> >> + } >> >> switch (type) { >> >> case 'c': >> >> res |= S_IFCHR; >> >> break; >> >> ... >> >> *rdev = MKDEV(major, minor); >> > >> > I think the plan 9 folk should figure out what's right. >> > >> > > > ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.clktrk
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index b352457..574095e 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -63,7 +63,7 @@ static const struct inode_operations v9fs_symlink_inode_operations; static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode) { - int res; + u32 res; res = mode & 0777; if (S_ISDIR(mode)) res |= P9_DMDIR; @@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses, static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses, struct p9_wstat *stat, dev_t *rdev) { - int res; + umode_t res; u32 mode = stat->mode; *rdev = 0; @@ -144,7 +144,7 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses, else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses)) && (v9ses->nodev == 0)) { char type = 0, ext[32]; - int major = -1, minor = -1; + u32 major = 0, minor = 0; strlcpy(ext, stat->extension, sizeof(ext)); sscanf(ext, "%c %u %u", &type, &major, &minor);
Changes the sign type to unsigned, avoiding the possibility of wrap when ORing the p9 or unix bit modes. Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> --- fs/9p/vfs_inode.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)