| Message ID | 20200107130441.y3owvcnxdljailt5@kili.mountain |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] cmd64x: potential buffer overflow in cmd64x_program_timings() | expand |
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Tue, 7 Jan 2020 16:04:41 +0300 > The "drive->dn" value is a u8 and it is controlled by root only, but > it could be out of bounds here so let's check. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Applied.
Hi, On 1/20/20 2:40 PM, David Miller wrote: > From: Dan Carpenter <dan.carpenter@oracle.com> > Date: Tue, 7 Jan 2020 16:04:41 +0300 > >> The "drive->dn" value is a u8 and it is controlled by root only, but >> it could be out of bounds here so let's check. drive->dn should not be root controllable, please point me where it happens as this may need fixing instead of cmd64x driver. [ IDE core makes sure that drive->dn is never > 3 and a lot of code assumes it. ] >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Applied. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Tue, Jan 21, 2020 at 12:15:54PM +0100, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > On 1/20/20 2:40 PM, David Miller wrote: > > From: Dan Carpenter <dan.carpenter@oracle.com> > > Date: Tue, 7 Jan 2020 16:04:41 +0300 > > > >> The "drive->dn" value is a u8 and it is controlled by root only, but > >> it could be out of bounds here so let's check. > > drive->dn should not be root controllable, please point me where it > happens as this may need fixing instead of cmd64x driver. > > [ IDE core makes sure that drive->dn is never > 3 and a lot of code > assumes it. ] > It's a marked as a setable field in ide-proc.c drivers/ide/ide-proc.c 206 ide_devset_rw(current_speed, xfer_rate); 207 ide_devset_rw_field(init_speed, init_speed); 208 ide_devset_rw_flag(nice1, IDE_DFLAG_NICE1); 209 ide_devset_rw_field(number, dn); ^^^^^^^^^^ Sets ->dn 210 211 static const struct ide_proc_devset ide_generic_settings[] = { 212 IDE_PROC_DEVSET(current_speed, 0, 70), 213 IDE_PROC_DEVSET(init_speed, 0, 70), 214 IDE_PROC_DEVSET(io_32bit, 0, 1 + (SUPPORT_VLB_SYNC << 1)), 215 IDE_PROC_DEVSET(keepsettings, 0, 1), 216 IDE_PROC_DEVSET(nice1, 0, 1), 217 IDE_PROC_DEVSET(number, 0, 3), 218 IDE_PROC_DEVSET(pio_mode, 0, 255), 219 IDE_PROC_DEVSET(unmaskirq, 0, 1), 220 IDE_PROC_DEVSET(using_dma, 0, 1), 221 { NULL }, 222 }; drivers/ide/ide-devsets.c 159 int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting, 160 int arg) 161 { 162 struct request_queue *q = drive->queue; 163 struct request *rq; 164 int ret = 0; 165 166 if (!(setting->flags & DS_SYNC)) 167 return setting->set(drive, arg); ^^^^^^^^^^^^^^^^^^^^^^^^ Called from here according to Smatch. 168 169 rq = blk_get_request(q, REQ_OP_DRV_IN, 0); regards, dan carpenter
On Tue, Jan 21, 2020 at 02:48:35PM +0300, Dan Carpenter wrote: > On Tue, Jan 21, 2020 at 12:15:54PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > Hi, > > > > On 1/20/20 2:40 PM, David Miller wrote: > > > From: Dan Carpenter <dan.carpenter@oracle.com> > > > Date: Tue, 7 Jan 2020 16:04:41 +0300 > > > > > >> The "drive->dn" value is a u8 and it is controlled by root only, but > > >> it could be out of bounds here so let's check. > > > > drive->dn should not be root controllable, please point me where it > > happens as this may need fixing instead of cmd64x driver. > > > > [ IDE core makes sure that drive->dn is never > 3 and a lot of code > > assumes it. ] > > > > It's a marked as a setable field in ide-proc.c > > drivers/ide/ide-proc.c > 206 ide_devset_rw(current_speed, xfer_rate); > 207 ide_devset_rw_field(init_speed, init_speed); > 208 ide_devset_rw_flag(nice1, IDE_DFLAG_NICE1); > 209 ide_devset_rw_field(number, dn); > ^^^^^^^^^^ > Sets ->dn > > 210 > 211 static const struct ide_proc_devset ide_generic_settings[] = { > 212 IDE_PROC_DEVSET(current_speed, 0, 70), > 213 IDE_PROC_DEVSET(init_speed, 0, 70), > 214 IDE_PROC_DEVSET(io_32bit, 0, 1 + (SUPPORT_VLB_SYNC << 1)), > 215 IDE_PROC_DEVSET(keepsettings, 0, 1), > 216 IDE_PROC_DEVSET(nice1, 0, 1), > 217 IDE_PROC_DEVSET(number, 0, 3), ^^^^ Argh... This clamps it to 0-3 doesn't it. Sorry, I didn't see that. regards, dan carpenter
On Tue, Jan 21, 2020 at 02:48:35PM +0300, Dan Carpenter wrote: > On Tue, Jan 21, 2020 at 12:15:54PM +0100, Bartlomiej Zolnierkiewicz wrote: > > > > Hi, > > > > On 1/20/20 2:40 PM, David Miller wrote: > > > From: Dan Carpenter <dan.carpenter@oracle.com> > > > Date: Tue, 7 Jan 2020 16:04:41 +0300 > > > > > >> The "drive->dn" value is a u8 and it is controlled by root only, but > > >> it could be out of bounds here so let's check. > > > > drive->dn should not be root controllable, please point me where it > > happens as this may need fixing instead of cmd64x driver. > > > > [ IDE core makes sure that drive->dn is never > 3 and a lot of code > > assumes it. ] > > > > It's a marked as a setable field in ide-proc.c > > drivers/ide/ide-proc.c > 206 ide_devset_rw(current_speed, xfer_rate); > 207 ide_devset_rw_field(init_speed, init_speed); > 208 ide_devset_rw_flag(nice1, IDE_DFLAG_NICE1); > 209 ide_devset_rw_field(number, dn); > ^^^^^^^^^^ > Sets ->dn > > 210 > 211 static const struct ide_proc_devset ide_generic_settings[] = { > 212 IDE_PROC_DEVSET(current_speed, 0, 70), > 213 IDE_PROC_DEVSET(init_speed, 0, 70), > 214 IDE_PROC_DEVSET(io_32bit, 0, 1 + (SUPPORT_VLB_SYNC << 1)), > 215 IDE_PROC_DEVSET(keepsettings, 0, 1), > 216 IDE_PROC_DEVSET(nice1, 0, 1), > 217 IDE_PROC_DEVSET(number, 0, 3), > 218 IDE_PROC_DEVSET(pio_mode, 0, 255), > 219 IDE_PROC_DEVSET(unmaskirq, 0, 1), > 220 IDE_PROC_DEVSET(using_dma, 0, 1), > 221 { NULL }, > 222 }; > > drivers/ide/ide-devsets.c > 159 int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting, > 160 int arg) > 161 { > 162 struct request_queue *q = drive->queue; > 163 struct request *rq; > 164 int ret = 0; > 165 > 166 if (!(setting->flags & DS_SYNC)) > 167 return setting->set(drive, arg); > ^^^^^^^^^^^^^^^^^^^^^^^^ > Called from here according to Smatch. > Actually this is where I went wrong. The function is never called from here. Sorry for the noise. These patches are not required. regards, dan carpenter
On 1/21/20 12:48 PM, Dan Carpenter wrote: > On Tue, Jan 21, 2020 at 12:15:54PM +0100, Bartlomiej Zolnierkiewicz wrote: >> >> Hi, >> >> On 1/20/20 2:40 PM, David Miller wrote: >>> From: Dan Carpenter <dan.carpenter@oracle.com> >>> Date: Tue, 7 Jan 2020 16:04:41 +0300 >>> >>>> The "drive->dn" value is a u8 and it is controlled by root only, but >>>> it could be out of bounds here so let's check. >> >> drive->dn should not be root controllable, please point me where it >> happens as this may need fixing instead of cmd64x driver. >> >> [ IDE core makes sure that drive->dn is never > 3 and a lot of code >> assumes it. ] >> > > It's a marked as a setable field in ide-proc.c > > drivers/ide/ide-proc.c > 206 ide_devset_rw(current_speed, xfer_rate); > 207 ide_devset_rw_field(init_speed, init_speed); > 208 ide_devset_rw_flag(nice1, IDE_DFLAG_NICE1); > 209 ide_devset_rw_field(number, dn); > ^^^^^^^^^^ > Sets ->dn It is a bug. We should add: #define ide_devset_ro_field(_name, _field) \ ide_devset_get(_name, _field); \ IDE_DEVSET(_name, 0, get_##_name, NULL) in <linux/ide.h> (just after ide_devset_rw_field()) and use it instead. Care to make a patch? > 210 > 211 static const struct ide_proc_devset ide_generic_settings[] = { > 212 IDE_PROC_DEVSET(current_speed, 0, 70), > 213 IDE_PROC_DEVSET(init_speed, 0, 70), > 214 IDE_PROC_DEVSET(io_32bit, 0, 1 + (SUPPORT_VLB_SYNC << 1)), > 215 IDE_PROC_DEVSET(keepsettings, 0, 1), > 216 IDE_PROC_DEVSET(nice1, 0, 1), > 217 IDE_PROC_DEVSET(number, 0, 3), Please note the minimum and maximum values above and look at code in ide_write_setting(): if ((ds->flags & DS_SYNC) && (val < setting->min || val > setting->max)) return -EINVAL; [ DS_SYNC flag is set by ide_devset_rw_field() macro. ] Thus even without fixing ide-proc.c both your patches are superfluous. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics > 218 IDE_PROC_DEVSET(pio_mode, 0, 255), > 219 IDE_PROC_DEVSET(unmaskirq, 0, 1), > 220 IDE_PROC_DEVSET(using_dma, 0, 1), > 221 { NULL }, > 222 }; > > drivers/ide/ide-devsets.c > 159 int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting, > 160 int arg) > 161 { > 162 struct request_queue *q = drive->queue; > 163 struct request *rq; > 164 int ret = 0; > 165 > 166 if (!(setting->flags & DS_SYNC)) > 167 return setting->set(drive, arg); > ^^^^^^^^^^^^^^^^^^^^^^^^ > Called from here according to Smatch. > > 168 > 169 rq = blk_get_request(q, REQ_OP_DRV_IN, 0); > > regards, > dan carpenter
On 1/21/20 1:21 PM, Bartlomiej Zolnierkiewicz wrote: > > On 1/21/20 12:48 PM, Dan Carpenter wrote: >> On Tue, Jan 21, 2020 at 12:15:54PM +0100, Bartlomiej Zolnierkiewicz wrote: >>> >>> Hi, >>> >>> On 1/20/20 2:40 PM, David Miller wrote: >>>> From: Dan Carpenter <dan.carpenter@oracle.com> >>>> Date: Tue, 7 Jan 2020 16:04:41 +0300 >>>> >>>>> The "drive->dn" value is a u8 and it is controlled by root only, but >>>>> it could be out of bounds here so let's check. >>> >>> drive->dn should not be root controllable, please point me where it >>> happens as this may need fixing instead of cmd64x driver. >>> >>> [ IDE core makes sure that drive->dn is never > 3 and a lot of code >>> assumes it. ] >>> >> >> It's a marked as a setable field in ide-proc.c >> >> drivers/ide/ide-proc.c >> 206 ide_devset_rw(current_speed, xfer_rate); >> 207 ide_devset_rw_field(init_speed, init_speed); >> 208 ide_devset_rw_flag(nice1, IDE_DFLAG_NICE1); >> 209 ide_devset_rw_field(number, dn); >> ^^^^^^^^^^ >> Sets ->dn > > It is a bug. PS The rationale for fixing it is: - IDE core always sets ->dn correctly (changing it is never required) - setting different value than assigned by IDE core is very likely to result in data corruption (due to wrong transfer timings being set on the controller etc.) Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
diff --git a/drivers/ide/cmd64x.c b/drivers/ide/cmd64x.c index a1898e11b04e..943bf944bf72 100644 --- a/drivers/ide/cmd64x.c +++ b/drivers/ide/cmd64x.c @@ -66,6 +66,9 @@ static void cmd64x_program_timings(ide_drive_t *drive, u8 mode) struct ide_timing t; u8 arttim = 0; + if (drive->dn >= ARRAY_SIZE(drwtim_regs)) + return; + ide_timing_compute(drive, mode, &t, T, 0); /*
The "drive->dn" value is a u8 and it is controlled by root only, but it could be out of bounds here so let's check. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/ide/cmd64x.c | 3 +++ 1 file changed, 3 insertions(+)