* [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c @ 2019-05-23 2:38 Gen Zhang 2019-06-05 6:00 ` Jiri Slaby 0 siblings, 1 reply; 4+ messages in thread From: Gen Zhang @ 2019-05-23 2:38 UTC (permalink / raw) To: martin.petersen; +Cc: linux-scsi, linux-kernel In sg_write(), the opcode of the command is fetched the first time from the userspace by __get_user(). Then the whole command, the opcode included, is fetched again from userspace by __copy_from_user(). However, a malicious user can change the opcode between the two fetches. This can cause inconsistent data and potential errors as cmnd is used in the following codes. Thus we should check opcode between the two fetches to prevent this. Signed-off-by: Gen Zhang <blackgod016574@gmail.com> --- diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index d3f1531..a2971b8 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) hp->flags = input_size; /* structure abuse ... */ hp->pack_id = old_hdr.pack_id; hp->usr_ptr = NULL; + if (opcode != cmnd[0]) + return -EINVAL; if (__copy_from_user(cmnd, buf, cmd_size)) return -EFAULT; /* --- ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c 2019-05-23 2:38 [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c Gen Zhang @ 2019-06-05 6:00 ` Jiri Slaby 2019-06-05 17:07 ` Douglas Gilbert 0 siblings, 1 reply; 4+ messages in thread From: Jiri Slaby @ 2019-06-05 6:00 UTC (permalink / raw) To: Gen Zhang, martin.petersen; +Cc: linux-scsi, linux-kernel On 23. 05. 19, 4:38, Gen Zhang wrote: > In sg_write(), the opcode of the command is fetched the first time from > the userspace by __get_user(). Then the whole command, the opcode > included, is fetched again from userspace by __copy_from_user(). > However, a malicious user can change the opcode between the two fetches. > This can cause inconsistent data and potential errors as cmnd is used in > the following codes. > > Thus we should check opcode between the two fetches to prevent this. > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > --- > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index d3f1531..a2971b8 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) > hp->flags = input_size; /* structure abuse ... */ > hp->pack_id = old_hdr.pack_id; > hp->usr_ptr = NULL; > + if (opcode != cmnd[0]) > + return -EINVAL; Isn't it too early to check cmnd which is copied only here: > if (__copy_from_user(cmnd, buf, cmd_size)) > return -EFAULT; > /* > --- > thanks, -- js suse labs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c 2019-06-05 6:00 ` Jiri Slaby @ 2019-06-05 17:07 ` Douglas Gilbert 2019-06-06 8:00 ` Gen Zhang 0 siblings, 1 reply; 4+ messages in thread From: Douglas Gilbert @ 2019-06-05 17:07 UTC (permalink / raw) To: Jiri Slaby, Gen Zhang, martin.petersen; +Cc: linux-scsi, linux-kernel On 2019-06-05 2:00 a.m., Jiri Slaby wrote: > On 23. 05. 19, 4:38, Gen Zhang wrote: >> In sg_write(), the opcode of the command is fetched the first time from >> the userspace by __get_user(). Then the whole command, the opcode >> included, is fetched again from userspace by __copy_from_user(). >> However, a malicious user can change the opcode between the two fetches. >> This can cause inconsistent data and potential errors as cmnd is used in >> the following codes. >> >> Thus we should check opcode between the two fetches to prevent this. >> >> Signed-off-by: Gen Zhang <blackgod016574@gmail.com> >> --- >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index d3f1531..a2971b8 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) >> hp->flags = input_size; /* structure abuse ... */ >> hp->pack_id = old_hdr.pack_id; >> hp->usr_ptr = NULL; >> + if (opcode != cmnd[0]) >> + return -EINVAL; > > Isn't it too early to check cmnd which is copied only here: > >> if (__copy_from_user(cmnd, buf, cmd_size)) >> return -EFAULT; >> /* >> --- >> Hi, Yes, it is too early. It needs to be after that __copy_from_user(cmnd, buf, cmd_size) call. To put this in context, this is a very old interface; dating from 1992 and deprecated for almost 20 years. The fact that the first byte of the SCSI cdb needs to be read first to work out that size of the following SCSI command and optionally the offset of a data-out buffer that may follow the command; is one reason why that interface was replaced. Also the implementation did not handle SCSI variable length cdb_s. Then there is the question of whether this double-fetch is exploitable? I cannot think of an example, but there might be (e.g. turning a READ command into a WRITE). But the "double-fetch" issue may be more wide spread. The replacement interface passes the command and data-in/-out as pointers while their corresponding lengths are placed in the newer interface structure. This assumes that the cdb and data-out won't change in the user space between when the write(2) is called and before or while the driver, using those pointers, reads the data. All drivers that use pointers to pass data have this "feature". Also I'm looking at this particular double-fetch from the point of view of the driver rewrite I have done and is currently in the early stages of review [linux-scsi list: "[PATCH 00/19] sg: v4 interface, rq sharing + multiple rqs"] and this problem is more difficult to fix since the full cdb read is delayed to a common point further along the submit processing path. To detect a change in cbd[0] my current code would need to be altered to carry cdb[0] through to that common point. So is it worth it for such an old, deprecated and replaced interface?? What cdb/user_permissions checking that is done, is done _after_ the full cdb is read. So trying to get around a user exclusion of say WRITE(10) by first using the first byte of READ(10), won't succeed. Doug Gilbert ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c 2019-06-05 17:07 ` Douglas Gilbert @ 2019-06-06 8:00 ` Gen Zhang 0 siblings, 0 replies; 4+ messages in thread From: Gen Zhang @ 2019-06-06 8:00 UTC (permalink / raw) To: Douglas Gilbert; +Cc: Jiri Slaby, martin.petersen, linux-scsi, linux-kernel On Wed, Jun 05, 2019 at 01:07:25PM -0400, Douglas Gilbert wrote: > On 2019-06-05 2:00 a.m., Jiri Slaby wrote: > >On 23. 05. 19, 4:38, Gen Zhang wrote: > >>In sg_write(), the opcode of the command is fetched the first time from > >>the userspace by __get_user(). Then the whole command, the opcode > >>included, is fetched again from userspace by __copy_from_user(). > >>However, a malicious user can change the opcode between the two fetches. > >>This can cause inconsistent data and potential errors as cmnd is used in > >>the following codes. > >> > >>Thus we should check opcode between the two fetches to prevent this. > >> > >>Signed-off-by: Gen Zhang <blackgod016574@gmail.com> > >>--- > >>diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > >>index d3f1531..a2971b8 100644 > >>--- a/drivers/scsi/sg.c > >>+++ b/drivers/scsi/sg.c > >>@@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos) > >> hp->flags = input_size; /* structure abuse ... */ > >> hp->pack_id = old_hdr.pack_id; > >> hp->usr_ptr = NULL; > >>+ if (opcode != cmnd[0]) > >>+ return -EINVAL; > > > >Isn't it too early to check cmnd which is copied only here: > > > >> if (__copy_from_user(cmnd, buf, cmd_size)) > >> return -EFAULT; > >> /* > >>--- > >> > > Hi, > Yes, it is too early. It needs to be after that __copy_from_user(cmnd, > buf, cmd_size) call. Yes, it is. > > To put this in context, this is a very old interface; dating from 1992 > and deprecated for almost 20 years. The fact that the first byte of > the SCSI cdb needs to be read first to work out that size of the > following SCSI command and optionally the offset of a data-out > buffer that may follow the command; is one reason why that interface > was replaced. Also the implementation did not handle SCSI variable > length cdb_s. > > Then there is the question of whether this double-fetch is exploitable? > I cannot think of an example, but there might be (e.g. turning a READ > command into a WRITE). But the "double-fetch" issue may be more wide > spread. The replacement interface passes the command and data-in/-out as > pointers while their corresponding lengths are placed in the newer > interface structure. This assumes that the cdb and data-out won't > change in the user space between when the write(2) is called and > before or while the driver, using those pointers, reads the data. > All drivers that use pointers to pass data have this "feature". > > Also I'm looking at this particular double-fetch from the point of view > of the driver rewrite I have done and is currently in the early stages > of review [linux-scsi list: "[PATCH 00/19] sg: v4 interface, rq sharing > + multiple rqs"] and this problem is more difficult to fix since the > full cdb read is delayed to a common point further along the submit > processing path. To detect a change in cbd[0] my current code would > need to be altered to carry cdb[0] through to that common point. So > is it worth it for such an old, deprecated and replaced interface?? > What cdb/user_permissions checking that is done, is done _after_ > the full cdb is read. So trying to get around a user exclusion of > say WRITE(10) by first using the first byte of READ(10), won't succeed. > > Doug Gilbert > Thanks for your explaination. I guess this patch should be dropped for the above reasons. Thanks Gen ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-06 8:01 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-23 2:38 [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c Gen Zhang 2019-06-05 6:00 ` Jiri Slaby 2019-06-05 17:07 ` Douglas Gilbert 2019-06-06 8:00 ` Gen Zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.