| Message ID | 1456871112-14103-1-git-send-email-jacob.e.keller@intel.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Mar 1, 2016 at 2:25 PM, Jacob Keller <jacob.e.keller@intel.com> wrote: > Since at least 2005, (oldest commit in ethtool.git), the userspace > ethtool implementation has given the size of the memory it has allocated > as the actual size in the ethtool data structures. We previously blindly > ignore this and overwrite the requested size with the current size > returned by .get_strings or .get_sset_count. This can cause problems if > these values aren't static. > > Since ethtool has always given the expected size, first perform a check > to ensure that the current size is no larger than the requested size. If > it is, return with -ENOMEM, as we do not have enough memory to save the > results. > > This protects against buffer overrun should the driver have a non-static > number of statistics, tests, or private flags, and the value changes > between the ethtool userspace call to .get_sset_count and the actual > calls to populate the requested memory. > > Update the header files to indicate the expected behavior of userspace. > This change shouldn't break current or previous userspace as they have > consistently included the length correctly since as far back as we can > check. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Reported-by: Alexander Duyck <alexander.duyck@gmail.com> > --- > > - v2 > * use -ENOSPC instead of -ENOMEM at the suggestion of Mark Rustad This still has the potential to provide garbage data. What you should probably do at each stage is make sure the length matches with the exact value that you would expect. I assume you cannot have any fields shuffle on you? What I mean by that is that you don't want to have a setup with 4 Tx and 4 Rx rings where you then replace it with 1 Tx and 7 Rx rings and try to populate the same data into a setup where the strings reported are for 4 Tx and 4 Rx. You should double check that the length can be used as a means of identifying exactly what strings will be where. - Alex
On Tue, 2016-03-01 at 14:31 -0800, Alexander Duyck wrote: > This still has the potential to provide garbage data. What you > should > probably do at each stage is make sure the length matches with the > exact value that you would expect. > Sure, an exact check could be done instead, however... > I assume you cannot have any fields shuffle on you? What I mean by > that is that you don't want to have a setup with 4 Tx and 4 Rx rings > where you then replace it with 1 Tx and 7 Rx rings and try to > populate > the same data into a setup where the strings reported are for 4 Tx > and > 4 Rx. You should double check that the length can be used as a means > of identifying exactly what strings will be where. > > - Alex Darn. Looks like you're right. It would be theoretically possible for the number of queues (or other variables) to change such that the size matches but the data no longer lines up against the strings. For queues, I don't think we're vulnerable on the fm10k driver, because we only use combined queues. However, we already have support for "debug-statistics" which shows extra stats plus some stats per virtual function. I am not sure if these could change within the time window to result in garbage data. I don't know how much of a real world problem this would be though. I'm guessing it's more reason to promote the idea of converting to some new tool based on netlink. Thanks, Jake
On Tue, Mar 1, 2016 at 2:58 PM, Keller, Jacob E <jacob.e.keller@intel.com> wrote: > On Tue, 2016-03-01 at 14:31 -0800, Alexander Duyck wrote: >> This still has the potential to provide garbage data. What you >> should >> probably do at each stage is make sure the length matches with the >> exact value that you would expect. >> > > Sure, an exact check could be done instead, however... > >> I assume you cannot have any fields shuffle on you? What I mean by >> that is that you don't want to have a setup with 4 Tx and 4 Rx rings >> where you then replace it with 1 Tx and 7 Rx rings and try to >> populate >> the same data into a setup where the strings reported are for 4 Tx >> and >> 4 Rx. You should double check that the length can be used as a means >> of identifying exactly what strings will be where. >> >> - Alex > > > Darn. Looks like you're right. It would be theoretically possible for > the number of queues (or other variables) to change such that the size > matches but the data no longer lines up against the strings. > > For queues, I don't think we're vulnerable on the fm10k driver, because > we only use combined queues. However, we already have support for > "debug-statistics" which shows extra stats plus some stats per virtual > function. I am not sure if these could change within the time window to > result in garbage data. > > I don't know how much of a real world problem this would be though. > > I'm guessing it's more reason to promote the idea of converting to some > new tool based on netlink. Yeah, we had basically pushed the issue under the rug by using a static layout for statistics. Was the motivation for getting rid of the static layout just to declutter the display? I know I have gotten into the habit of just piping ethtool -S through a "| grep -v :\ 0" to drop any of the unused stats from the display. Also you may see issues depending on how the stats might be parsed by user-space scripts if you start modifying what is displayed and what is not. - Alex
On Tue, 2016-03-01 at 15:05 -0800, Alexander Duyck wrote: > On Tue, Mar 1, 2016 at 2:58 PM, Keller, Jacob E > <jacob.e.keller@intel.com> wrote: > > > > On Tue, 2016-03-01 at 14:31 -0800, Alexander Duyck wrote: > > > > > > This still has the potential to provide garbage data. What you > > > should > > > probably do at each stage is make sure the length matches with > > > the > > > exact value that you would expect. > > > > > Sure, an exact check could be done instead, however... > > > > > > > > I assume you cannot have any fields shuffle on you? What I mean > > > by > > > that is that you don't want to have a setup with 4 Tx and 4 Rx > > > rings > > > where you then replace it with 1 Tx and 7 Rx rings and try to > > > populate > > > the same data into a setup where the strings reported are for 4 > > > Tx > > > and > > > 4 Rx. You should double check that the length can be used as a > > > means > > > of identifying exactly what strings will be where. > > > > > > - Alex > > > > Darn. Looks like you're right. It would be theoretically possible > > for > > the number of queues (or other variables) to change such that the > > size > > matches but the data no longer lines up against the strings. > > > > For queues, I don't think we're vulnerable on the fm10k driver, > > because > > we only use combined queues. However, we already have support for > > "debug-statistics" which shows extra stats plus some stats per > > virtual > > function. I am not sure if these could change within the time > > window to > > result in garbage data. > > > > I don't know how much of a real world problem this would be though. > > > > I'm guessing it's more reason to promote the idea of converting to > > some > > new tool based on netlink. > Yeah, we had basically pushed the issue under the rug by using a > static layout for statistics. > Yep. > Was the motivation for getting rid of the static layout just to > declutter the display? I know I have gotten into the habit of just > piping ethtool -S through a "| grep -v :\ 0" to drop any of the > unused > stats from the display. Also you may see issues depending on how the > stats might be parsed by user-space scripts if you start modifying > what is displayed and what is not. > Generally, I'm not too concerned about specific user-scripts parsing since these already have to be heavily dependent on the driver in question (since each driver has their own naming scheme). I added a while back the private flag "debug-statistics" to enable a bunch of extra statistics which weren't always relevant. In some cases, like the queue size change it was done in order to simply remove clutter of 128 queueus when many systems would only use a few of those. Just doing grep -v :\ 0 will remove these lines, true. However, that doesn't help the debug-statistics case, where we have a ton of statistics which may be relevant to some debugging, but aren't really useful to see all the time. In addition, it also shows stats for VFs in this mode as well. Thanks, Jake
On Tue, 2016-03-01 at 14:25 -0800, Jacob Keller wrote: > Since at least 2005, (oldest commit in ethtool.git), the userspace > ethtool implementation has given the size of the memory it has allocated > as the actual size in the ethtool data structures. We previously blindly > ignore this and overwrite the requested size with the current size > returned by .get_strings or .get_sset_count. This can cause problems if > these values aren't static. [...] NAK, ethtool is not the only consumer of the ethtool API. How many times do I have to repeat myself? Ben.
On Wed, 2016-03-02 at 00:12 +0000, Ben Hutchings wrote: > NAK, ethtool is not the only consumer of the ethtool API. How many > times do I have to repeat myself? > > Ben. > Ok, so essentially forcing drivers to require static sets for the various stats/strings/etc? :( Will have patches to cleanup fm10k for this then. Regards, Jake
On Wed, 2016-03-02 at 01:11 +0000, Keller, Jacob E wrote: > On Wed, 2016-03-02 at 00:12 +0000, Ben Hutchings wrote: > > NAK, ethtool is not the only consumer of the ethtool API. How many > > times do I have to repeat myself? > > > > Ben. > > > > Ok, so essentially forcing drivers to require static sets for the > various stats/strings/etc? No, you can define a new and better statistics API. There are many limitations of the current API that could be addressed at the same time, e.g. lack of units, lack of hierarchy in naming, lack of distinction between counters and other statistics. Ben. > :( > > Will have patches to cleanup fm10k for this then. > > Regards, > Jake
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 37fd6dc33de4..0d7f4855dc0b 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -560,13 +560,16 @@ enum ethtool_stringset { * struct ethtool_gstrings - string set for data tagging * @cmd: Command number = %ETHTOOL_GSTRINGS * @string_set: String set ID; one of &enum ethtool_stringset - * @len: On return, the number of strings in the string set + * @len: On entry, the number of strings which fit into the allocated space. + * On return, the number of strings in the string set * @data: Buffer for strings. Each string is null-padded to a size of * %ETH_GSTRING_LEN. * * Users must use %ETHTOOL_GSSET_INFO to find the number of strings in * the string set. They must allocate a buffer of the appropriate - * size immediately following this structure. + * size immediately following this structure. They must set the len to the + * number of strings which will fit into the allocated space following this + * structure. */ struct ethtool_gstrings { __u32 cmd; @@ -622,13 +625,15 @@ enum ethtool_test_flags { * @flags: A bitmask of flags from &enum ethtool_test_flags. Some * flags may be set by the user on entry; others may be set by * the driver on return. - * @len: On return, the number of test results + * @len: On entry, the number of test results that fit in the allocated space + * On return, the number of test results * @data: Array of test results * * Users must use %ETHTOOL_GSSET_INFO or %ETHTOOL_GDRVINFO to find the * number of test results that will be returned. They must allocate a * buffer of the appropriate size (8 * number of results) immediately - * following this structure. + * following this structure. They must set the len to the number of tests + * results which will fit into the allocated space following the structure. */ struct ethtool_test { __u32 cmd; @@ -641,13 +646,15 @@ struct ethtool_test { /** * struct ethtool_stats - device-specific statistics * @cmd: Command number = %ETHTOOL_GSTATS - * @n_stats: On return, the number of statistics + * @n_stats: On entry, the number of stats that fit into the allocated space. + * On return, the number of statistics * @data: Array of statistics * * Users must use %ETHTOOL_GSSET_INFO or %ETHTOOL_GDRVINFO to find the * number of statistics that will be returned. They must allocate a * buffer of the appropriate size (8 * number of statistics) - * immediately following this structure. + * immediately following this structure. They must set n_stats to the number + * of stats which fit into the allocated space at the end of the structure. */ struct ethtool_stats { __u32 cmd; diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 2966cd0d7c93..8ce7d1b5756d 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1766,6 +1766,9 @@ static int ethtool_self_test(struct net_device *dev, char __user *useraddr) if (copy_from_user(&test, useraddr, sizeof(test))) return -EFAULT; + if (test_len > test.len) + return -ENOSPC; + test.len = test_len; data = kmalloc(test_len * sizeof(u64), GFP_USER); if (!data) @@ -1799,6 +1802,9 @@ static int ethtool_get_strings(struct net_device *dev, void __user *useraddr) if (ret < 0) return ret; + if (ret > gstrings.len) + return -ENOSPC; + gstrings.len = ret; data = kcalloc(gstrings.len, ETH_GSTRING_LEN, GFP_USER); @@ -1898,6 +1904,9 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr) if (copy_from_user(&stats, useraddr, sizeof(stats))) return -EFAULT; + if (n_stats > stats.n_stats) + return -ENOSPC; + stats.n_stats = n_stats; data = kmalloc(n_stats * sizeof(u64), GFP_USER); if (!data) @@ -1937,6 +1946,9 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr) if (copy_from_user(&stats, useraddr, sizeof(stats))) return -EFAULT; + if (n_stats > stats.n_stats) + return -ENOSPC; + stats.n_stats = n_stats; data = kmalloc_array(n_stats, sizeof(u64), GFP_USER); if (!data)
Since at least 2005, (oldest commit in ethtool.git), the userspace ethtool implementation has given the size of the memory it has allocated as the actual size in the ethtool data structures. We previously blindly ignore this and overwrite the requested size with the current size returned by .get_strings or .get_sset_count. This can cause problems if these values aren't static. Since ethtool has always given the expected size, first perform a check to ensure that the current size is no larger than the requested size. If it is, return with -ENOMEM, as we do not have enough memory to save the results. This protects against buffer overrun should the driver have a non-static number of statistics, tests, or private flags, and the value changes between the ethtool userspace call to .get_sset_count and the actual calls to populate the requested memory. Update the header files to indicate the expected behavior of userspace. This change shouldn't break current or previous userspace as they have consistently included the length correctly since as far back as we can check. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Reported-by: Alexander Duyck <alexander.duyck@gmail.com> --- - v2 * use -ENOSPC instead of -ENOMEM at the suggestion of Mark Rustad include/uapi/linux/ethtool.h | 19 +++++++++++++------ net/core/ethtool.c | 12 ++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-)