PATCH: Off-by-one bug in user page calculations for Direct I/O

PATCH: Off-by-one bug in user page calculations for Direct I/O

[Alan Stern]

The page count calculations in drivers/scsi/st.c (and copied in sg.c) are
wrong.  The code says:

	nr_pages = ((uaddr & ~PAGE_MASK) + count - 1 + ~PAGE_MASK) >> 
		PAGE_SHIFT;

That will compute an incorrect value if the user's buffer happens to end 
on the first byte of a new page.  Example:  Suppose uaddr starts right on 
a page boundary and count is PAGE_SIZE + 1.  Then

	(uaddr & ~PAGE_MASK)	->  0
	count - 1		->  PAGE_SIZE
	~PAGE_MASK		->  PAGE_SIZE - 1

	nr_pages		-> (2 * PAGE_SIZE - 1) >> PAGE_SHIFT = 1

when in fact nr_pages should be 2.  Either the "- 1" shouldn't be there or 
the second "~PAGE_MASK" should be replaced by "PAGE_SIZE".

Alan Stern


--- a/drivers/scsi/st.c	Fri Sep  5 13:58:01 2003
+++ b/drivers/scsi/st.c	Sun Nov 16 11:44:47 2003
@@ -4036,7 +4036,7 @@
 	unsigned int nr_pages;
 	struct page **pages;
 
-	nr_pages = ((uaddr & ~PAGE_MASK) + count - 1 + ~PAGE_MASK) >> PAGE_SHIFT;
+	nr_pages = ((uaddr & ~PAGE_MASK) + count + ~PAGE_MASK) >> PAGE_SHIFT;
 
 	/* User attempted Overflow! */
 	if ((uaddr + count) < uaddr)
--- a/drivers/scsi/sg.c	Mon Oct 20 10:19:01 2003
+++ b/drivers/scsi/sg.c	Sun Nov 16 11:45:10 2003
@@ -1625,7 +1625,7 @@
 	unsigned int nr_pages;
 	struct page **pages;
 
-	nr_pages = ((uaddr & ~PAGE_MASK) + count - 1 + ~PAGE_MASK) >> PAGE_SHIFT;
+	nr_pages = ((uaddr & ~PAGE_MASK) + count + ~PAGE_MASK) >> PAGE_SHIFT;
 
 	/* User attempted Overflow! */
 	if ((uaddr + count) < uaddr)

Re: PATCH: Off-by-one bug in user page calculations for Direct I/O

[Jens Axboe]

On Sun, Nov 16 2003, Alan Stern wrote:
> The page count calculations in drivers/scsi/st.c (and copied in sg.c) are
> wrong.  The code says:
> 
> 	nr_pages = ((uaddr & ~PAGE_MASK) + count - 1 + ~PAGE_MASK) >> 
> 		PAGE_SHIFT;
> 
> That will compute an incorrect value if the user's buffer happens to end 
> on the first byte of a new page.  Example:  Suppose uaddr starts right on 
> a page boundary and count is PAGE_SIZE + 1.  Then
> 
> 	(uaddr & ~PAGE_MASK)	->  0
> 	count - 1		->  PAGE_SIZE
> 	~PAGE_MASK		->  PAGE_SIZE - 1
> 
> 	nr_pages		-> (2 * PAGE_SIZE - 1) >> PAGE_SHIFT = 1
> 
> when in fact nr_pages should be 2.  Either the "- 1" shouldn't be there or 
> the second "~PAGE_MASK" should be replaced by "PAGE_SIZE".

Good catch, that's a classic error. page calculations 101? :)

-- 
Jens Axboe

Re: PATCH: Off-by-one bug in user page calculations for Direct I/O

[Alan Stern]

James:

Please apply the patch below to the scsi-bugfixes-2.6 tree.

Alan Stern


On Sun, 16 Nov 2003, Kai Makisara wrote:

> On Sun, 16 Nov 2003, Alan Stern wrote:
> 
> > The page count calculations in drivers/scsi/st.c (and copied in sg.c) are
> > wrong.  The code says:
> >
> > 	nr_pages = ((uaddr & ~PAGE_MASK) + count - 1 + ~PAGE_MASK) >>
> > 		PAGE_SHIFT;
> >
> > That will compute an incorrect value if the user's buffer happens to end
> > on the first byte of a new page.  Example:  Suppose uaddr starts right on
> 
> Your analysis is correct and this is a bug. Could you send the fix to
> James Bottomley for inclusion into the scsi-bugfixes-2.6 bk tree (at least
> the st part).
> 
> Thanks for noticing the bug.
> 
> 	Kai
> 
> P.S. I usually write these ((base ~ mask) + count + PAGE_SIZE - 1) >>
> PAGE_SHIFT. I don't know why I did it like this here. One should never try
> to be clever and do something in a new way or copy something that does not
> match one's own standard ways of doing things ;-)

On Mon, 17 Nov 2003, Douglas Gilbert wrote:

> Alan,
> ... and the sg part as well ..
> 
> > Thanks for noticing the bug.
> 
> dito
> 
> Doug Gilbert


===== sg.c 1.48 vs edited =====
--- 1.48/drivers/scsi/sg.c	Fri Oct 24 14:53:37 2003
+++ edited/drivers/scsi/sg.c	Mon Nov 17 09:57:36 2003
@@ -1627,7 +1627,7 @@
 	unsigned int nr_pages;
 	struct page **pages;
 
-	nr_pages = ((uaddr & ~PAGE_MASK) + count - 1 + ~PAGE_MASK) >> PAGE_SHIFT;
+	nr_pages = ((uaddr & ~PAGE_MASK) + count + ~PAGE_MASK) >> PAGE_SHIFT;
 
 	/* User attempted Overflow! */
 	if ((uaddr + count) < uaddr)
===== st.c 1.45 vs edited =====
--- 1.45/drivers/scsi/st.c	Fri Sep  5 12:16:40 2003
+++ edited/drivers/scsi/st.c	Mon Nov 17 09:57:09 2003
@@ -4036,7 +4036,7 @@
 	unsigned int nr_pages;
 	struct page **pages;
 
-	nr_pages = ((uaddr & ~PAGE_MASK) + count - 1 + ~PAGE_MASK) >> PAGE_SHIFT;
+	nr_pages = ((uaddr & ~PAGE_MASK) + count + ~PAGE_MASK) >> PAGE_SHIFT;
 
 	/* User attempted Overflow! */
 	if ((uaddr + count) < uaddr)