[PATCH 1/2] Ensure nul-termination of file names read from checkpoint images

[PATCH 1/2] Ensure nul-termination of file names read from checkpoint images

[Matt Helsley]

Don't rely on the checkpoint image to properly terminate the filename.
Passing PATH_MAX + 1 won't work since it's a maximum -- not the number
of bytes to allocate. Allocate space for the string, copy an amount
according to the header length (limited to < PATH_MAX), and ensure that
it's nul-terminated.

Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/files.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/checkpoint/files.c b/checkpoint/files.c
index f6de07e..0564666 100644
--- a/checkpoint/files.c
+++ b/checkpoint/files.c
@@ -443,6 +443,7 @@ struct file *restore_open_fname(struct ckpt_ctx *ctx, int flags)
 	struct ckpt_hdr *h;
 	struct file *file;
 	char *fname;
+	int len;
 
 	/* prevent bad input from doing bad things */
 	if (flags & (O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC))
@@ -451,10 +452,19 @@ struct file *restore_open_fname(struct ckpt_ctx *ctx, int flags)
 	h = ckpt_read_buf_type(ctx, PATH_MAX, CKPT_HDR_FILE_NAME);
 	if (IS_ERR(h))
 		return (struct file *) h;
-	fname = (char *) (h + 1);
+	len = h->len - sizeof(*h);
+	fname = kmalloc(len + 1, GFP_KERNEL);
+	if (!fname) {
+		file = NULL;
+		goto out;
+	}
+	strncpy(fname, (char *) (h + 1), len);
+	fname[len] = '\0';
 	ckpt_debug("fname '%s' flags %#x\n", fname, flags);
 
 	file = filp_open(fname, flags, 0);
+	kfree(fname);
+out:
 	ckpt_hdr_put(ctx, h);
 
 	return file;
-- 
1.5.6.3

[PATCH 2/2] File name length limit off by sizeof(struct ckpt_hdr)

[Matt Helsley]

Unlike the length passed into ckpt_write_obj_type, the maximum length passed
to ckpt_read_buf_type must include the length of the struct ckpt_hdr.

Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/files.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/checkpoint/files.c b/checkpoint/files.c
index 0564666..562c338 100644
--- a/checkpoint/files.c
+++ b/checkpoint/files.c
@@ -449,7 +449,7 @@ struct file *restore_open_fname(struct ckpt_ctx *ctx, int flags)
 	if (flags & (O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC))
 		return ERR_PTR(-EINVAL);
 
-	h = ckpt_read_buf_type(ctx, PATH_MAX, CKPT_HDR_FILE_NAME);
+	h = ckpt_read_buf_type(ctx, PATH_MAX + sizeof(*h), CKPT_HDR_FILE_NAME);
 	if (IS_ERR(h))
 		return (struct file *) h;
 	len = h->len - sizeof(*h);
-- 
1.5.6.3

Re: [PATCH 2/2] File name length limit off by sizeof(struct ckpt_hdr)

[Oren Laadan]

Matt Helsley wrote:
> Unlike the length passed into ckpt_write_obj_type, the maximum length passed
> to ckpt_read_buf_type must include the length of the struct ckpt_hdr.

IMHO, the right way to fix this is to change ckpt_read_obj_type().

This will preserve symmetry between checkpoint and restart, and also
fix a similar problem in kernel/groups.c (MAX_GROUPINFO_SIZE).

No need to resend - I'll fix already.

Oren.

> 
> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/files.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/checkpoint/files.c b/checkpoint/files.c
> index 0564666..562c338 100644
> --- a/checkpoint/files.c
> +++ b/checkpoint/files.c
> @@ -449,7 +449,7 @@ struct file *restore_open_fname(struct ckpt_ctx *ctx, int flags)
>  	if (flags & (O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC))
>  		return ERR_PTR(-EINVAL);
>  
> -	h = ckpt_read_buf_type(ctx, PATH_MAX, CKPT_HDR_FILE_NAME);
> +	h = ckpt_read_buf_type(ctx, PATH_MAX + sizeof(*h), CKPT_HDR_FILE_NAME);
>  	if (IS_ERR(h))
>  		return (struct file *) h;
>  	len = h->len - sizeof(*h);

Re: [PATCH 2/2] File name length limit off by sizeof(struct ckpt_hdr)

[Matt Helsley]

On Fri, Oct 23, 2009 at 08:29:13PM -0400, Oren Laadan wrote:
> 
> 
> Matt Helsley wrote:
> > Unlike the length passed into ckpt_write_obj_type, the maximum length passed
> > to ckpt_read_buf_type must include the length of the struct ckpt_hdr.
> 
> IMHO, the right way to fix this is to change ckpt_read_obj_type().

Good point.

> This will preserve symmetry between checkpoint and restart, and also
> fix a similar problem in kernel/groups.c (MAX_GROUPINFO_SIZE).
> 
> No need to resend - I'll fix already.

Good, thanks!

Cheers,
	-Matt Helsley

Re: [PATCH 2/2] File name length limit off by sizeof(struct ckpt_hdr)

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
> 
> 
> Matt Helsley wrote:
> > Unlike the length passed into ckpt_write_obj_type, the maximum length passed
> > to ckpt_read_buf_type must include the length of the struct ckpt_hdr.
> 
> IMHO, the right way to fix this is to change ckpt_read_obj_type().
> 
> This will preserve symmetry between checkpoint and restart, and also
> fix a similar problem in kernel/groups.c (MAX_GROUPINFO_SIZE).
> 
> No need to resend - I'll fix already.

Oren: note with your version of the patch, restore_open_fname()
does 'return len' giving me

checkpoint/files.c: In function 'restore_open_fname':
checkpoint/files.c:457: warning: return makes pointer from integer without a cast

-serge

Re: [PATCH 2/2] File name length limit off by sizeof(struct ckpt_hdr)

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
>>
>> Matt Helsley wrote:
>>> Unlike the length passed into ckpt_write_obj_type, the maximum length passed
>>> to ckpt_read_buf_type must include the length of the struct ckpt_hdr.
>> IMHO, the right way to fix this is to change ckpt_read_obj_type().
>>
>> This will preserve symmetry between checkpoint and restart, and also
>> fix a similar problem in kernel/groups.c (MAX_GROUPINFO_SIZE).
>>
>> No need to resend - I'll fix already.
> 
> Oren: note with your version of the patch, restore_open_fname()
> does 'return len' giving me
> 
> checkpoint/files.c: In function 'restore_open_fname':
> checkpoint/files.c:457: warning: return makes pointer from integer without a cast

Oops ... that's silly. Fix queued.

Thanks,

Oren.

Re: [PATCH 1/2] Ensure nul-termination of file names read from checkpoint images

[Oren Laadan]

Matt Helsley wrote:
> Don't rely on the checkpoint image to properly terminate the filename.
> Passing PATH_MAX + 1 won't work since it's a maximum -- not the number
> of bytes to allocate. Allocate space for the string, copy an amount
> according to the header length (limited to < PATH_MAX), and ensure that
> it's nul-terminated.
> 
> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

I dislike unneeded data copy.
See ckpt_read_string() and ckpt_read_payload().

Oren.

> ---
>  checkpoint/files.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/checkpoint/files.c b/checkpoint/files.c
> index f6de07e..0564666 100644
> --- a/checkpoint/files.c
> +++ b/checkpoint/files.c
> @@ -443,6 +443,7 @@ struct file *restore_open_fname(struct ckpt_ctx *ctx, int flags)
>  	struct ckpt_hdr *h;
>  	struct file *file;
>  	char *fname;
> +	int len;
>  
>  	/* prevent bad input from doing bad things */
>  	if (flags & (O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC))
> @@ -451,10 +452,19 @@ struct file *restore_open_fname(struct ckpt_ctx *ctx, int flags)
>  	h = ckpt_read_buf_type(ctx, PATH_MAX, CKPT_HDR_FILE_NAME);
>  	if (IS_ERR(h))
>  		return (struct file *) h;
> -	fname = (char *) (h + 1);
> +	len = h->len - sizeof(*h);
> +	fname = kmalloc(len + 1, GFP_KERNEL);
> +	if (!fname) {
> +		file = NULL;
> +		goto out;
> +	}
> +	strncpy(fname, (char *) (h + 1), len);
> +	fname[len] = '\0';
>  	ckpt_debug("fname '%s' flags %#x\n", fname, flags);
>  
>  	file = filp_open(fname, flags, 0);
> +	kfree(fname);
> +out:
>  	ckpt_hdr_put(ctx, h);
>  
>  	return file;

Re: [PATCH 1/2] Ensure nul-termination of file names read from checkpoint images

[Oren Laadan]

Oren Laadan wrote:
> 
> Matt Helsley wrote:
>> Don't rely on the checkpoint image to properly terminate the filename.
>> Passing PATH_MAX + 1 won't work since it's a maximum -- not the number
>> of bytes to allocate. Allocate space for the string, copy an amount
>> according to the header length (limited to < PATH_MAX), and ensure that
>> it's nul-terminated.
>>
>> Signed-off-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> I dislike unneeded data copy.
> See ckpt_read_string() and ckpt_read_payload().
> 

No need to resend -- I'll do the fix.

Oren.