* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
2020-01-21 11:48 ` Dan Carpenter
@ 2020-01-21 11:55 ` Dan Carpenter
2020-01-21 12:07 ` Dan Carpenter
2020-01-21 12:21 ` Bartlomiej Zolnierkiewicz
2 siblings, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-01-21 11:55 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: David Miller, linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
2020-01-21 11:48 ` Dan Carpenter
2020-01-21 11:55 ` Dan Carpenter
@ 2020-01-21 12:07 ` Dan Carpenter
2020-01-21 12:21 ` Bartlomiej Zolnierkiewicz
2 siblings, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-01-21 12:07 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: David Miller, linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
2020-01-21 11:48 ` Dan Carpenter
2020-01-21 11:55 ` Dan Carpenter
2020-01-21 12:07 ` Dan Carpenter
@ 2020-01-21 12:21 ` Bartlomiej Zolnierkiewicz
2020-01-21 12:38 ` Bartlomiej Zolnierkiewicz
2 siblings, 1 reply; 15+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-21 12:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David Miller, linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
2020-01-21 12:21 ` Bartlomiej Zolnierkiewicz
@ 2020-01-21 12:38 ` Bartlomiej Zolnierkiewicz
2020-01-21 13:06 ` [PATCH] ide: make drive->dn read only Dan Carpenter
0 siblings, 1 reply; 15+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-21 12:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David Miller, linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ide: make drive->dn read only
2020-01-21 12:38 ` Bartlomiej Zolnierkiewicz
@ 2020-01-21 13:06 ` Dan Carpenter
2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
2020-01-30 10:03 ` David Miller
0 siblings, 2 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-01-21 13:06 UTC (permalink / raw)
To: David S. Miller, Bartlomiej Zolnierkiewicz; +Cc: linux-ide, kernel-janitors
The IDE core always sets ->dn correctly so changing it is never
required.
Setting it to a 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.)
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
include/linux/ide.h | 4 ++++
drivers/ide/ide-proc.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 06dae6438557..a254841bd315 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -945,6 +945,10 @@ ide_devset_get(_name, _field); \
ide_devset_set(_name, _field); \
IDE_DEVSET(_name, DS_SYNC, get_##_name, set_##_name)
+#define ide_devset_ro_field(_name, _field) \
+ide_devset_get(_name, _field); \
+IDE_DEVSET(_name, 0, get_##_name, NULL)
+
#define ide_devset_rw_flag(_name, _field) \
ide_devset_get_flag(_name, _field); \
ide_devset_set_flag(_name, _field); \
diff --git a/drivers/ide/ide-proc.c b/drivers/ide/ide-proc.c
index 11a801aa92d8..15c17f3781ee 100644
--- a/drivers/ide/ide-proc.c
+++ b/drivers/ide/ide-proc.c
@@ -206,7 +206,7 @@ static int set_xfer_rate (ide_drive_t *drive, int arg)
ide_devset_rw(current_speed, xfer_rate);
ide_devset_rw_field(init_speed, init_speed);
ide_devset_rw_flag(nice1, IDE_DFLAG_NICE1);
-ide_devset_rw_field(number, dn);
+ide_devset_ro_field(number, dn);
static const struct ide_proc_devset ide_generic_settings[] = {
IDE_PROC_DEVSET(current_speed, 0, 70),
--
2.11.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH] ide: make drive->dn read only
2020-01-21 13:06 ` [PATCH] ide: make drive->dn read only Dan Carpenter
@ 2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
2020-01-21 14:17 ` Dan Carpenter
2020-01-30 10:03 ` David Miller
1 sibling, 1 reply; 15+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-21 14:13 UTC (permalink / raw)
To: Dan Carpenter, David S. Miller; +Cc: linux-ide, kernel-janitors
On 1/21/20 2:06 PM, Dan Carpenter wrote:
> The IDE core always sets ->dn correctly so changing it is never
> required.
>
> Setting it to a 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.)
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Thanks, it looks fine (though patch summary can be improved further i.e.:
"[PATCH] ide-proc: make "number" setting read-only").
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
I've also verified it (using ARAnyM emulator):
Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> ---
>
> include/linux/ide.h | 4 ++++
> drivers/ide/ide-proc.c | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/ide.h b/include/linux/ide.h
> index 06dae6438557..a254841bd315 100644
> --- a/include/linux/ide.h
> +++ b/include/linux/ide.h
> @@ -945,6 +945,10 @@ ide_devset_get(_name, _field); \
> ide_devset_set(_name, _field); \
> IDE_DEVSET(_name, DS_SYNC, get_##_name, set_##_name)
>
> +#define ide_devset_ro_field(_name, _field) \
> +ide_devset_get(_name, _field); \
> +IDE_DEVSET(_name, 0, get_##_name, NULL)
> +
> #define ide_devset_rw_flag(_name, _field) \
> ide_devset_get_flag(_name, _field); \
> ide_devset_set_flag(_name, _field); \
> diff --git a/drivers/ide/ide-proc.c b/drivers/ide/ide-proc.c
> index 11a801aa92d8..15c17f3781ee 100644
> --- a/drivers/ide/ide-proc.c
> +++ b/drivers/ide/ide-proc.c
> @@ -206,7 +206,7 @@ static int set_xfer_rate (ide_drive_t *drive, int arg)
> ide_devset_rw(current_speed, xfer_rate);
> ide_devset_rw_field(init_speed, init_speed);
> ide_devset_rw_flag(nice1, IDE_DFLAG_NICE1);
> -ide_devset_rw_field(number, dn);
> +ide_devset_ro_field(number, dn);
>
> static const struct ide_proc_devset ide_generic_settings[] = {
> IDE_PROC_DEVSET(current_speed, 0, 70),
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] ide: make drive->dn read only
2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
@ 2020-01-21 14:17 ` Dan Carpenter
0 siblings, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-01-21 14:17 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: David S. Miller, linux-ide, kernel-janitors
On Tue, Jan 21, 2020 at 03:13:29PM +0100, Bartlomiej Zolnierkiewicz wrote:
>
> On 1/21/20 2:06 PM, Dan Carpenter wrote:
> > The IDE core always sets ->dn correctly so changing it is never
> > required.
> >
> > Setting it to a 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.)
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Thanks, it looks fine (though patch summary can be improved further i.e.:
> "[PATCH] ide-proc: make "number" setting read-only").
>
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>
> I've also verified it (using ARAnyM emulator):
>
> Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>
Thanks!
regards,
dan carpenter
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ide: make drive->dn read only
2020-01-21 13:06 ` [PATCH] ide: make drive->dn read only Dan Carpenter
2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
@ 2020-01-30 10:03 ` David Miller
1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2020-01-30 10:03 UTC (permalink / raw)
To: dan.carpenter; +Cc: b.zolnierkie, linux-ide, kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 21 Jan 2020 16:06:42 +0300
> The IDE core always sets ->dn correctly so changing it is never
> required.
>
> Setting it to a 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.)
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied, thanks Dan.
^ permalink raw reply [flat|nested] 15+ messages in thread