* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <20170215112015.GA27080-2UyX9mZUyMU@public.gmane.org> @ 2017-02-15 11:22 ` Cyril Hrubis [not found] ` <20170215112205.GA27269-2UyX9mZUyMU@public.gmane.org> 2017-04-10 15:21 ` Michael Kerrisk (man-pages) 1 sibling, 1 reply; 8+ messages in thread From: Cyril Hrubis @ 2017-02-15 11:22 UTC (permalink / raw) To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w Cc: linux-man-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA [CCing linux api as well] > The BLKRASET/BLKRAGET ioctls() take unsigned long, if I pass int * to > the BLKRAGET ioctl on x86_64 (or on any other arch where sizeof(int) != > sizeof(long)) the BLKRAGET ioctl will rewrite four bytes on the stack. > > If you look at block/ioctl.c in kernel sources you can clearly see that > BLKRAGET ioctl calls put_long(). > > Compile following reproducer and run it as ./a.out /dev/sda, you can see > that the second member of the array will be zeroed. If you change the > array to have only one member you will see stack smashing trace. > > I also wonder if it's OK to pass int value to ioctl() at all, the arg > value seems to be unsigned long in the syscall definition in fs/ioctl.c > and there does not seem to be any glibc magic around the syscall. > > -------------------------8<---------------------------- > #include <sys/mount.h> > #include <sys/ioctl.h> > #include <fcntl.h> > #include <stdio.h> > > static int fd; > > int main(int argc, char *argv[]) > { > int ra[] = {100, 100}; > > fd = open(argv[1], O_RDONLY); > if (fd < 0) { > perror("open"); > return 1; > } > > ioctl(fd, BLKRAGET, ra); > > fprintf(stderr, "%i %i\n", ra[0], ra[1]); > > return 0; > } > > -------------------------8<---------------------------- > > Signed-off-by: Cyril Hrubis <chrubis-AlSwsSmVLrQ@public.gmane.org> > --- > man2/ioctl_list.2 | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/man2/ioctl_list.2 b/man2/ioctl_list.2 > index 0165c77..c8efd66 100644 > --- a/man2/ioctl_list.2 > +++ b/man2/ioctl_list.2 > @@ -311,8 +311,8 @@ l l l l. > 0x0000125F BLKRRPART void > 0x00001260 BLKGETSIZE unsigned long * > 0x00001261 BLKFLSBUF void > -0x00001262 BLKRASET int > -0x00001263 BLKRAGET int * > +0x00001262 BLKRASET unsigned long > +0x00001263 BLKRAGET unsigned long * > 0x00000001 FIBMAP int * // I-O > 0x00000002 FIGETBSZ int * > 0x80086601 FS_IOC_GETFLAGS int * > -- > 2.10.2 > > > -- > Cyril Hrubis > chrubis-AlSwsSmVLrQ@public.gmane.org -- Cyril Hrubis chrubis-AlSwsSmVLrQ@public.gmane.org ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20170215112205.GA27269-2UyX9mZUyMU@public.gmane.org>]
* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <20170215112205.GA27269-2UyX9mZUyMU@public.gmane.org> @ 2017-02-15 12:04 ` Arnd Bergmann [not found] ` <CAK8P3a0KzhqFZqvPH4q7_Nb6+TMmhCWXDi_-wQG=mi-1U=Ccxw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-03-14 13:21 ` Cyril Hrubis 1 sibling, 1 reply; 8+ messages in thread From: Arnd Bergmann @ 2017-02-15 12:04 UTC (permalink / raw) To: Cyril Hrubis Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA On Wed, Feb 15, 2017 at 12:22 PM, Cyril Hrubis <chrubis-AlSwsSmVLrQ@public.gmane.org> wrote: > [CCing linux api as well] >> The BLKRASET/BLKRAGET ioctls() take unsigned long, if I pass int * to >> the BLKRAGET ioctl on x86_64 (or on any other arch where sizeof(int) != >> sizeof(long)) the BLKRAGET ioctl will rewrite four bytes on the stack. >> >> If you look at block/ioctl.c in kernel sources you can clearly see that >> BLKRAGET ioctl calls put_long(). BLKFRAGET as well, fwiw, see compat_blkdev_ioctl(). We might as well include them in the list. >> I also wonder if it's OK to pass int value to ioctl() at all, the arg >> value seems to be unsigned long in the syscall definition in fs/ioctl.c >> and there does not seem to be any glibc magic around the syscall. This shouldn't matter, if you pass an 'int' into a function that takes a 'long', it will be extended if necessary. The question is more about how it gets interpreted, and in this case it's done by assigning to struct backing_dev_info { ... unsigned long ra_pages; /* max readahead in PAGE_SIZE units */ ... }; Large values will not be used in practice (these are capped by the I/O sizes), but I guess you can set a value larger than UINT_MAX and read it back later. >> man2/ioctl_list.2 | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/man2/ioctl_list.2 b/man2/ioctl_list.2 >> index 0165c77..c8efd66 100644 >> --- a/man2/ioctl_list.2 >> +++ b/man2/ioctl_list.2 >> @@ -311,8 +311,8 @@ l l l l. >> 0x0000125F BLKRRPART void >> 0x00001260 BLKGETSIZE unsigned long * >> 0x00001261 BLKFLSBUF void >> -0x00001262 BLKRASET int >> -0x00001263 BLKRAGET int * >> +0x00001262 BLKRASET unsigned long >> +0x00001263 BLKRAGET unsigned long * >> 0x00000001 FIBMAP int * // I-O >> 0x00000002 FIGETBSZ int * >> 0x80086601 FS_IOC_GETFLAGS int * Looks correct to me. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CAK8P3a0KzhqFZqvPH4q7_Nb6+TMmhCWXDi_-wQG=mi-1U=Ccxw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <CAK8P3a0KzhqFZqvPH4q7_Nb6+TMmhCWXDi_-wQG=mi-1U=Ccxw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2017-02-15 12:59 ` Cyril Hrubis [not found] ` <20170215125946.GA27511-2UyX9mZUyMU@public.gmane.org> 2017-04-10 15:21 ` Michael Kerrisk (man-pages) 1 sibling, 1 reply; 8+ messages in thread From: Cyril Hrubis @ 2017-02-15 12:59 UTC (permalink / raw) To: Arnd Bergmann Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA Hi! > >> I also wonder if it's OK to pass int value to ioctl() at all, the arg > >> value seems to be unsigned long in the syscall definition in fs/ioctl.c > >> and there does not seem to be any glibc magic around the syscall. > > This shouldn't matter, if you pass an 'int' into a function that takes > a 'long', it will be extended if necessary. The question is more about > how it gets interpreted, and in this case it's done by assigning to I guess that it may, since the ioctl() prototype is defined with ... >From /usr/include/sys/ioctl.h: ... extern int ioctl (int __fd, unsigned long int __request, ...) __THROW; ... Looking at dissasembly we do: 4005bb: 89 c7 mov %eax,%edi (%eax holds fd from open()) ... 4005bb: 89 c7 mov %eax,%edi 4005bd: ba 00 02 00 00 mov $0x200,%edx ^ We do 32bit load here 4005c2: be 62 12 00 00 mov $0x1262,%esi 4005c7: 31 c0 xor %eax,%eax 4005c9: e8 72 ff ff ff callq 400540 <ioctl@plt> The ioctl assembly just sets %eax and then syscall. So as far as I can tell we depend here on a fact that upper part of $rdx is zeroed. Not sure if that is guaranteed or not. -- Cyril Hrubis chrubis-AlSwsSmVLrQ@public.gmane.org -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20170215125946.GA27511-2UyX9mZUyMU@public.gmane.org>]
* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <20170215125946.GA27511-2UyX9mZUyMU@public.gmane.org> @ 2017-02-15 14:29 ` Cyril Hrubis 0 siblings, 0 replies; 8+ messages in thread From: Cyril Hrubis @ 2017-02-15 14:29 UTC (permalink / raw) To: Arnd Bergmann Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA Hi! > So as far as I can tell we depend here on a fact that upper part of $rdx > is zeroed. Not sure if that is guaranteed or not. Which seems to be the case for x86_64 processors, so passing int value to ioctl() will work fine there. Not sure about the rest of 64bit archs. -- Cyril Hrubis chrubis-AlSwsSmVLrQ@public.gmane.org -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <CAK8P3a0KzhqFZqvPH4q7_Nb6+TMmhCWXDi_-wQG=mi-1U=Ccxw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-02-15 12:59 ` Cyril Hrubis @ 2017-04-10 15:21 ` Michael Kerrisk (man-pages) 1 sibling, 0 replies; 8+ messages in thread From: Michael Kerrisk (man-pages) @ 2017-04-10 15:21 UTC (permalink / raw) To: Arnd Bergmann, Cyril Hrubis Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA On 02/15/2017 01:04 PM, Arnd Bergmann wrote: > On Wed, Feb 15, 2017 at 12:22 PM, Cyril Hrubis <chrubis-AlSwsSmVLrQ@public.gmane.org> wrote: >> [CCing linux api as well] >>> The BLKRASET/BLKRAGET ioctls() take unsigned long, if I pass int * to >>> the BLKRAGET ioctl on x86_64 (or on any other arch where sizeof(int) != >>> sizeof(long)) the BLKRAGET ioctl will rewrite four bytes on the stack. >>> >>> If you look at block/ioctl.c in kernel sources you can clearly see that >>> BLKRAGET ioctl calls put_long(). > > BLKFRAGET as well, fwiw, see compat_blkdev_ioctl(). > We might as well include them in the list. > >>> I also wonder if it's OK to pass int value to ioctl() at all, the arg >>> value seems to be unsigned long in the syscall definition in fs/ioctl.c >>> and there does not seem to be any glibc magic around the syscall. > > This shouldn't matter, if you pass an 'int' into a function that takes > a 'long', it will be extended if necessary. The question is more about > how it gets interpreted, and in this case it's done by assigning to > > struct backing_dev_info { > ... > unsigned long ra_pages; /* max readahead in PAGE_SIZE units */ > ... > }; > > Large values will not be used in practice (these are capped by the > I/O sizes), but I guess you can set a value larger than UINT_MAX > and read it back later. > >>> man2/ioctl_list.2 | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/man2/ioctl_list.2 b/man2/ioctl_list.2 >>> index 0165c77..c8efd66 100644 >>> --- a/man2/ioctl_list.2 >>> +++ b/man2/ioctl_list.2 >>> @@ -311,8 +311,8 @@ l l l l. >>> 0x0000125F BLKRRPART void >>> 0x00001260 BLKGETSIZE unsigned long * >>> 0x00001261 BLKFLSBUF void >>> -0x00001262 BLKRASET int >>> -0x00001263 BLKRAGET int * >>> +0x00001262 BLKRASET unsigned long >>> +0x00001263 BLKRAGET unsigned long * >>> 0x00000001 FIBMAP int * // I-O >>> 0x00000002 FIGETBSZ int * >>> 0x80086601 FS_IOC_GETFLAGS int * > > Looks correct to me. Thanks for reviewing this, Arnd. Cheers, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <20170215112205.GA27269-2UyX9mZUyMU@public.gmane.org> 2017-02-15 12:04 ` Arnd Bergmann @ 2017-03-14 13:21 ` Cyril Hrubis [not found] ` <20170314132120.GA8347-2UyX9mZUyMU@public.gmane.org> 1 sibling, 1 reply; 8+ messages in thread From: Cyril Hrubis @ 2017-03-14 13:21 UTC (permalink / raw) To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w Cc: linux-man-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA Hi! Ping. -- Cyril Hrubis chrubis-AlSwsSmVLrQ@public.gmane.org ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20170314132120.GA8347-2UyX9mZUyMU@public.gmane.org>]
* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <20170314132120.GA8347-2UyX9mZUyMU@public.gmane.org> @ 2017-04-03 14:02 ` Cyril Hrubis 0 siblings, 0 replies; 8+ messages in thread From: Cyril Hrubis @ 2017-04-03 14:02 UTC (permalink / raw) To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w Cc: linux-man-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA Hi! Ping. -- Cyril Hrubis chrubis-AlSwsSmVLrQ@public.gmane.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long [not found] ` <20170215112015.GA27080-2UyX9mZUyMU@public.gmane.org> 2017-02-15 11:22 ` [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long Cyril Hrubis @ 2017-04-10 15:21 ` Michael Kerrisk (man-pages) 1 sibling, 0 replies; 8+ messages in thread From: Michael Kerrisk (man-pages) @ 2017-04-10 15:21 UTC (permalink / raw) To: Cyril Hrubis Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-man-u79uwXL29TY76Z2rM5mHXA, Linux API On 02/15/2017 12:20 PM, Cyril Hrubis wrote: > The BLKRASET/BLKRAGET ioctls() take unsigned long, if I pass int * to > the BLKRAGET ioctl on x86_64 (or on any other arch where sizeof(int) != > sizeof(long)) the BLKRAGET ioctl will rewrite four bytes on the stack. > > If you look at block/ioctl.c in kernel sources you can clearly see that > BLKRAGET ioctl calls put_long(). > > Compile following reproducer and run it as ./a.out /dev/sda, you can see > that the second member of the array will be zeroed. If you change the > array to have only one member you will see stack smashing trace. > > I also wonder if it's OK to pass int value to ioctl() at all, the arg > value seems to be unsigned long in the syscall definition in fs/ioctl.c > and there does not seem to be any glibc magic around the syscall. Thanks Cyril. Applied now. Sorry for the delayed response... Cheers, Michael > -------------------------8<---------------------------- > #include <sys/mount.h> > #include <sys/ioctl.h> > #include <fcntl.h> > #include <stdio.h> > > static int fd; > > int main(int argc, char *argv[]) > { > int ra[] = {100, 100}; > > fd = open(argv[1], O_RDONLY); > if (fd < 0) { > perror("open"); > return 1; > } > > ioctl(fd, BLKRAGET, ra); > > fprintf(stderr, "%i %i\n", ra[0], ra[1]); > > return 0; > } > > -------------------------8<---------------------------- > > Signed-off-by: Cyril Hrubis <chrubis-AlSwsSmVLrQ@public.gmane.org> > --- > man2/ioctl_list.2 | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/man2/ioctl_list.2 b/man2/ioctl_list.2 > index 0165c77..c8efd66 100644 > --- a/man2/ioctl_list.2 > +++ b/man2/ioctl_list.2 > @@ -311,8 +311,8 @@ l l l l. > 0x0000125F BLKRRPART void > 0x00001260 BLKGETSIZE unsigned long * > 0x00001261 BLKFLSBUF void > -0x00001262 BLKRASET int > -0x00001263 BLKRAGET int * > +0x00001262 BLKRASET unsigned long > +0x00001263 BLKRAGET unsigned long * > 0x00000001 FIBMAP int * // I-O > 0x00000002 FIGETBSZ int * > 0x80086601 FS_IOC_GETFLAGS int * > -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-10 15:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20170215112015.GA27080@rei.lan> [not found] ` <20170215112015.GA27080-2UyX9mZUyMU@public.gmane.org> 2017-02-15 11:22 ` [PATCH] ioctl_list.2: BLKRASET/BLKRAGET take unsigned long Cyril Hrubis [not found] ` <20170215112205.GA27269-2UyX9mZUyMU@public.gmane.org> 2017-02-15 12:04 ` Arnd Bergmann [not found] ` <CAK8P3a0KzhqFZqvPH4q7_Nb6+TMmhCWXDi_-wQG=mi-1U=Ccxw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-02-15 12:59 ` Cyril Hrubis [not found] ` <20170215125946.GA27511-2UyX9mZUyMU@public.gmane.org> 2017-02-15 14:29 ` Cyril Hrubis 2017-04-10 15:21 ` Michael Kerrisk (man-pages) 2017-03-14 13:21 ` Cyril Hrubis [not found] ` <20170314132120.GA8347-2UyX9mZUyMU@public.gmane.org> 2017-04-03 14:02 ` Cyril Hrubis 2017-04-10 15:21 ` Michael Kerrisk (man-pages)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).