* [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-07 13:04 ` Dan Carpenter
0 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-01-07 13:04 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-ide, kernel-janitors
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(+)
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);
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-07 13:04 ` Dan Carpenter
0 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-01-07 13:04 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-ide, kernel-janitors
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(+)
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);
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] ide: serverworks: potential overflow in svwks_set_pio_mode()
2020-01-07 13:04 ` Dan Carpenter
@ 2020-01-07 13:06 ` Dan Carpenter
-1 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-01-07 13:06 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-ide, kernel-janitors
The "drive->dn" variable is a u8 controlled by root.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/ide/serverworks.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/ide/serverworks.c b/drivers/ide/serverworks.c
index ac6fc3fffa0d..458e72e034b0 100644
--- a/drivers/ide/serverworks.c
+++ b/drivers/ide/serverworks.c
@@ -115,6 +115,9 @@ static void svwks_set_pio_mode(ide_hwif_t *hwif, ide_drive_t *drive)
struct pci_dev *dev = to_pci_dev(hwif->dev);
const u8 pio = drive->pio_mode - XFER_PIO_0;
+ if (drive->dn >= ARRAY_SIZE(drive_pci))
+ return;
+
pci_write_config_byte(dev, drive_pci[drive->dn], pio_modes[pio]);
if (svwks_csb_check(dev)) {
@@ -141,6 +144,9 @@ static void svwks_set_dma_mode(ide_hwif_t *hwif, ide_drive_t *drive)
u8 ultra_enable = 0, ultra_timing = 0, dma_timing = 0;
+ if (drive->dn >= ARRAY_SIZE(drive_pci2))
+ return;
+
pci_read_config_byte(dev, (0x56|hwif->channel), &ultra_timing);
pci_read_config_byte(dev, 0x54, &ultra_enable);
--
2.11.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] ide: serverworks: potential overflow in svwks_set_pio_mode()
@ 2020-01-07 13:06 ` Dan Carpenter
0 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-01-07 13:06 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-ide, kernel-janitors
The "drive->dn" variable is a u8 controlled by root.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/ide/serverworks.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/ide/serverworks.c b/drivers/ide/serverworks.c
index ac6fc3fffa0d..458e72e034b0 100644
--- a/drivers/ide/serverworks.c
+++ b/drivers/ide/serverworks.c
@@ -115,6 +115,9 @@ static void svwks_set_pio_mode(ide_hwif_t *hwif, ide_drive_t *drive)
struct pci_dev *dev = to_pci_dev(hwif->dev);
const u8 pio = drive->pio_mode - XFER_PIO_0;
+ if (drive->dn >= ARRAY_SIZE(drive_pci))
+ return;
+
pci_write_config_byte(dev, drive_pci[drive->dn], pio_modes[pio]);
if (svwks_csb_check(dev)) {
@@ -141,6 +144,9 @@ static void svwks_set_dma_mode(ide_hwif_t *hwif, ide_drive_t *drive)
u8 ultra_enable = 0, ultra_timing = 0, dma_timing = 0;
+ if (drive->dn >= ARRAY_SIZE(drive_pci2))
+ return;
+
pci_read_config_byte(dev, (0x56|hwif->channel), &ultra_timing);
pci_read_config_byte(dev, 0x54, &ultra_enable);
--
2.11.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
2020-01-07 13:04 ` Dan Carpenter
@ 2020-01-20 13:40 ` David Miller
-1 siblings, 0 replies; 30+ messages in thread
From: David Miller @ 2020-01-20 13:40 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-ide, kernel-janitors
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-20 13:40 ` David Miller
0 siblings, 0 replies; 30+ messages in thread
From: David Miller @ 2020-01-20 13:40 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-ide, kernel-janitors
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] ide: serverworks: potential overflow in svwks_set_pio_mode()
2020-01-07 13:06 ` Dan Carpenter
@ 2020-01-20 13:40 ` David Miller
-1 siblings, 0 replies; 30+ messages in thread
From: David Miller @ 2020-01-20 13:40 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-ide, kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 7 Jan 2020 16:06:07 +0300
> The "drive->dn" variable is a u8 controlled by root.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] ide: serverworks: potential overflow in svwks_set_pio_mode()
@ 2020-01-20 13:40 ` David Miller
0 siblings, 0 replies; 30+ messages in thread
From: David Miller @ 2020-01-20 13:40 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-ide, kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 7 Jan 2020 16:06:07 +0300
> The "drive->dn" variable is a u8 controlled by root.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
[not found] ` <CGME20200121111555eucas1p2d26b5230ab428f40ee3fc76d687e601f@eucas1p2.samsung.com>
@ 2020-01-21 11:15 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 30+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-21 11:15 UTC (permalink / raw)
To: David Miller, dan.carpenter; +Cc: linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-21 11:15 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 30+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-21 11:15 UTC (permalink / raw)
To: David Miller, dan.carpenter; +Cc: linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Re: [PATCH 2/2] ide: serverworks: potential overflow in svwks_set_pio_mode()
[not found] ` <CGME20200121111816eucas1p246ca07e30a984c51d8281a161978d3ea@eucas1p2.samsung.com>
@ 2020-01-21 11:18 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 30+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-21 11:18 UTC (permalink / raw)
To: David Miller, dan.carpenter; +Cc: linux-ide, kernel-janitors
On 1/20/20 2:40 PM, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Tue, 7 Jan 2020 16:06:07 +0300
>
>> The "drive->dn" variable is a u8 controlled by root.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
drive->dn should not be root controllable, please point me where it
happens as this may need fixing instead of serverworks driver.
[ IDE core makes sure that drive->dn is never > 3 and a lot of code
assumes it. ]
> Applied.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Re: [PATCH 2/2] ide: serverworks: potential overflow in svwks_set_pio_mode()
@ 2020-01-21 11:18 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 30+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-01-21 11:18 UTC (permalink / raw)
To: David Miller, dan.carpenter; +Cc: linux-ide, kernel-janitors
On 1/20/20 2:40 PM, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Tue, 7 Jan 2020 16:06:07 +0300
>
>> The "drive->dn" variable is a u8 controlled by root.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
drive->dn should not be root controllable, please point me where it
happens as this may need fixing instead of serverworks driver.
[ IDE core makes sure that drive->dn is never > 3 and a lot of code
assumes it. ]
> Applied.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
2020-01-21 11:15 ` Bartlomiej Zolnierkiewicz
@ 2020-01-21 11:48 ` Dan Carpenter
-1 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-01-21 11:48 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: David Miller, linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-21 11:48 ` Dan Carpenter
0 siblings, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2020-01-21 11:48 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz; +Cc: David Miller, linux-ide, kernel-janitors
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
^ permalink raw reply [flat|nested] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-21 11:55 ` Dan Carpenter
0 siblings, 0 replies; 30+ 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] 30+ 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 12:07 ` Dan Carpenter
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-21 12:07 ` Dan Carpenter
0 siblings, 0 replies; 30+ 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] 30+ 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 12:21 ` Bartlomiej Zolnierkiewicz
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-21 12:21 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings()
@ 2020-01-21 12:38 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
* [PATCH] ide: make drive->dn read only
2020-01-21 12:38 ` Bartlomiej Zolnierkiewicz
@ 2020-01-21 13:06 ` Dan Carpenter
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* [PATCH] ide: make drive->dn read only
@ 2020-01-21 13:06 ` Dan Carpenter
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH] ide: make drive->dn read only
2020-01-21 13:06 ` Dan Carpenter
@ 2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH] ide: make drive->dn read only
@ 2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH] ide: make drive->dn read only
@ 2020-01-21 14:17 ` Dan Carpenter
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH] ide: make drive->dn read only
2020-01-21 13:06 ` Dan Carpenter
@ 2020-01-30 10:03 ` David Miller
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH] ide: make drive->dn read only
@ 2020-01-30 10:03 ` David Miller
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
end of thread, other threads:[~2020-01-30 10:03 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-07 13:04 [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings() Dan Carpenter
2020-01-07 13:04 ` Dan Carpenter
2020-01-07 13:06 ` [PATCH 2/2] ide: serverworks: potential overflow in svwks_set_pio_mode() Dan Carpenter
2020-01-07 13:06 ` Dan Carpenter
2020-01-20 13:40 ` David Miller
2020-01-20 13:40 ` David Miller
[not found] ` <CGME20200121111816eucas1p246ca07e30a984c51d8281a161978d3ea@eucas1p2.samsung.com>
2020-01-21 11:18 ` Bartlomiej Zolnierkiewicz
2020-01-21 11:18 ` Bartlomiej Zolnierkiewicz
2020-01-20 13:40 ` [PATCH 1/2] cmd64x: potential buffer overflow in cmd64x_program_timings() David Miller
2020-01-20 13:40 ` David Miller
[not found] ` <CGME20200121111555eucas1p2d26b5230ab428f40ee3fc76d687e601f@eucas1p2.samsung.com>
2020-01-21 11:15 ` Bartlomiej Zolnierkiewicz
2020-01-21 11:15 ` Bartlomiej Zolnierkiewicz
2020-01-21 11:48 ` Dan Carpenter
2020-01-21 11:48 ` Dan Carpenter
2020-01-21 11:55 ` Dan Carpenter
2020-01-21 11:55 ` Dan Carpenter
2020-01-21 12:07 ` Dan Carpenter
2020-01-21 12:07 ` Dan Carpenter
2020-01-21 12:21 ` Bartlomiej Zolnierkiewicz
2020-01-21 12:21 ` Bartlomiej Zolnierkiewicz
2020-01-21 12:38 ` Bartlomiej Zolnierkiewicz
2020-01-21 12:38 ` Bartlomiej Zolnierkiewicz
2020-01-21 13:06 ` [PATCH] ide: make drive->dn read only Dan Carpenter
2020-01-21 13:06 ` Dan Carpenter
2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
2020-01-21 14:13 ` Bartlomiej Zolnierkiewicz
2020-01-21 14:17 ` Dan Carpenter
2020-01-21 14:17 ` Dan Carpenter
2020-01-30 10:03 ` David Miller
2020-01-30 10:03 ` David Miller
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.