Re: splat in ikheaders_read (bpftrace)

[Kees Cook <keescook@xxxxxxxxxxxx>]

On Thu, Mar 02, 2023 at 02:12:31PM -0800, Jakub Kicinski wrote:
> On Thu, 2 Mar 2023 14:08:14 -0800 Jakub Kicinski wrote:
> > > static ssize_t
> > > ikheaders_read(struct file *file,  struct kobject *kobj,
> > > 	       struct bin_attribute *bin_attr,
> > > 	       char *buf, loff_t off, size_t len)
> > > {
> > > 	memcpy(buf, &kernel_headers_data + off, len);
> > > 	return len;
> > > }
> > > 
> > > I will take a look at the caller's allocation of "buf" and kernel_headers_data.  
> > 
> > Mm. Actually stopping to look at the code - I don't see it bound
> > checking against kernel_headers_data_end :| Maybe we need:
> > 
> > @@ -34,6 +35,7 @@ ikheaders_read(struct file *file,  struct kobject *kobj,
> >                struct bin_attribute *bin_attr,
> >                char *buf, loff_t off, size_t len)
> >  {
> > +       len = min_t(size_t, kernel_headers_data_end - kernel_headers_data, len);
> >         memcpy(buf, &kernel_headers_data + off, len);
> >         return len;
> >  }
> 
> Scratch that, the size is set at init time.
> My guess was memcpy() thinks the size of kernel_headers_data
> is 1 since it's declared as char?

I've improved the reporting, and yeah, it's due to the declaration:

  memcpy: detected buffer overflow: 4096 byte read from buffer of size 1

But that's an easy fix -- this has been done in lots of other places. It
needs to be an array, not a single char. (I'm surprised we hadn't seen
this before.)

diff --git a/kernel/kheaders.c b/kernel/kheaders.c
index 8f69772af77b..42163c9e94e5 100644
--- a/kernel/kheaders.c
+++ b/kernel/kheaders.c
@@ -26,15 +26,15 @@ asm (
 "	.popsection				\n"
 );
 
-extern char kernel_headers_data;
-extern char kernel_headers_data_end;
+extern char kernel_headers_data[];
+extern char kernel_headers_data_end[];
 
 static ssize_t
 ikheaders_read(struct file *file,  struct kobject *kobj,
 	       struct bin_attribute *bin_attr,
 	       char *buf, loff_t off, size_t len)
 {
-	memcpy(buf, &kernel_headers_data + off, len);
+	memcpy(buf, &kernel_headers_data[off], len);
 	return len;
 }
 
@@ -48,8 +48,8 @@ static struct bin_attribute kheaders_attr __ro_after_init = {
 
 static int __init ikheaders_init(void)
 {
-	kheaders_attr.size = (&kernel_headers_data_end -
-			      &kernel_headers_data);
+	kheaders_attr.size = (kernel_headers_data_end -
+			      kernel_headers_data);
 	return sysfs_create_bin_file(kernel_kobj, &kheaders_attr);
 }
 


-- 
Kees Cook