Re: drivers/s390/char/keyboard.c kernel stack infoleak
On 05.08.2017 03:57, sohu0106 wrote: > My idea is > > struct kbdiacr { > unsigned char diacr, base, result; > }; > > sizeof(struct kbdiacr)=4 > > here we just set 3 bytes > case KDGKBDIACR: > { > struct kbdiacrs __user *a = argp; > struct kbdiacr diacr; > int i; > > if (put_user(kbd->accent_table_size, >kb_cnt)) > return -EFAULT; > for (i = 0; i < kbd->accent_table_size; i++) { > diacr.diacr = kbd->accent_table[i].diacr; > diacr.base = kbd->accent_table[i].base; > diacr.result = kbd->accent_table[i].result; > if (copy_to_user(a->kbdiacr + i, , sizeof(struct kbdiacr))) > Is there anything I haven't noticed? Yes: sizeof(struct kbdiacr) is 3 here. Thomas
Re: drivers/s390/char/keyboard.c kernel stack infoleak
On 05.08.2017 03:57, sohu0106 wrote: > My idea is > > struct kbdiacr { > unsigned char diacr, base, result; > }; > > sizeof(struct kbdiacr)=4 > > here we just set 3 bytes > case KDGKBDIACR: > { > struct kbdiacrs __user *a = argp; > struct kbdiacr diacr; > int i; > > if (put_user(kbd->accent_table_size, >kb_cnt)) > return -EFAULT; > for (i = 0; i < kbd->accent_table_size; i++) { > diacr.diacr = kbd->accent_table[i].diacr; > diacr.base = kbd->accent_table[i].base; > diacr.result = kbd->accent_table[i].result; > if (copy_to_user(a->kbdiacr + i, , sizeof(struct kbdiacr))) > Is there anything I haven't noticed? Yes: sizeof(struct kbdiacr) is 3 here. Thomas
Re: drivers/s390/char/keyboard.c kernel stack infoleak
On Fri, Aug 04, 2017 at 11:09:24AM +0200, Thomas Huth wrote: > Hi, > > On 03.08.2017 15:59, sohu0106 wrote: > > > > The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes > > are padding bytes after "result" which are not initialized and leaked to > > userland via "copy_to_user". > > > > > > diff --git a/keyboard.c b/keyboard.c > > index ba0e4f9..76a6d35 100644 > > --- a/keyboard.c > > +++ b/keyboard.c > > @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, > > unsigned long arg) > > struct kbdiacr diacr; > > int i; > > > > + memset( , 0, sizeof(struct kbdiacr) ); > > + > > I think it would be nicer to simply init the struct with "= {}" > directly, i.e.: > > struct kbdiacr diacr = {}; > > And by the way, please have a look at the kernel patch submission > guidelines first, especially the COO part here: The patch doesn't make sense. There is no padding and therefore no information leak.
Re: drivers/s390/char/keyboard.c kernel stack infoleak
On Fri, Aug 04, 2017 at 11:09:24AM +0200, Thomas Huth wrote: > Hi, > > On 03.08.2017 15:59, sohu0106 wrote: > > > > The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes > > are padding bytes after "result" which are not initialized and leaked to > > userland via "copy_to_user". > > > > > > diff --git a/keyboard.c b/keyboard.c > > index ba0e4f9..76a6d35 100644 > > --- a/keyboard.c > > +++ b/keyboard.c > > @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, > > unsigned long arg) > > struct kbdiacr diacr; > > int i; > > > > + memset( , 0, sizeof(struct kbdiacr) ); > > + > > I think it would be nicer to simply init the struct with "= {}" > directly, i.e.: > > struct kbdiacr diacr = {}; > > And by the way, please have a look at the kernel patch submission > guidelines first, especially the COO part here: The patch doesn't make sense. There is no padding and therefore no information leak.
Re: drivers/s390/char/keyboard.c kernel stack infoleak
Hi, On 03.08.2017 15:59, sohu0106 wrote: > > The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes are > padding bytes after "result" which are not initialized and leaked to userland > via "copy_to_user". > > > diff --git a/keyboard.c b/keyboard.c > index ba0e4f9..76a6d35 100644 > --- a/keyboard.c > +++ b/keyboard.c > @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, > unsigned long arg) > struct kbdiacr diacr; > int i; > > + memset( , 0, sizeof(struct kbdiacr) ); > + I think it would be nicer to simply init the struct with "= {}" directly, i.e.: struct kbdiacr diacr = {}; And by the way, please have a look at the kernel patch submission guidelines first, especially the COO part here: https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin So looking at your patch, please try to send plain text mails, and sign your patch with a Signed-off-by line (i.e. no anonymous contributions). Thanks! Thomas
Re: drivers/s390/char/keyboard.c kernel stack infoleak
Hi, On 03.08.2017 15:59, sohu0106 wrote: > > The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes are > padding bytes after "result" which are not initialized and leaked to userland > via "copy_to_user". > > > diff --git a/keyboard.c b/keyboard.c > index ba0e4f9..76a6d35 100644 > --- a/keyboard.c > +++ b/keyboard.c > @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, > unsigned long arg) > struct kbdiacr diacr; > int i; > > + memset( , 0, sizeof(struct kbdiacr) ); > + I think it would be nicer to simply init the struct with "= {}" directly, i.e.: struct kbdiacr diacr = {}; And by the way, please have a look at the kernel patch submission guidelines first, especially the COO part here: https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin So looking at your patch, please try to send plain text mails, and sign your patch with a Signed-off-by line (i.e. no anonymous contributions). Thanks! Thomas
drivers/s390/char/keyboard.c kernel stack infoleak
The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes are padding bytes after "result" which are not initialized and leaked to userland via "copy_to_user". diff --git a/keyboard.c b/keyboard.c index ba0e4f9..76a6d35 100644 --- a/keyboard.c +++ b/keyboard.c @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, unsigned long arg) struct kbdiacr diacr; int i; + memset( , 0, sizeof(struct kbdiacr) ); + if (put_user(kbd->accent_table_size, >kb_cnt)) return -EFAULT; for (i = 0; i < kbd->accent_table_size; i++) {
drivers/s390/char/keyboard.c kernel stack infoleak
The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes are padding bytes after "result" which are not initialized and leaked to userland via "copy_to_user". diff --git a/keyboard.c b/keyboard.c index ba0e4f9..76a6d35 100644 --- a/keyboard.c +++ b/keyboard.c @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, unsigned long arg) struct kbdiacr diacr; int i; + memset( , 0, sizeof(struct kbdiacr) ); + if (put_user(kbd->accent_table_size, >kb_cnt)) return -EFAULT; for (i = 0; i < kbd->accent_table_size; i++) {